Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(DomActions) WTR tests for elementVisible() #497

Merged
merged 3 commits into from
Oct 24, 2024

Conversation

freiondrej
Copy link
Contributor

@freiondrej freiondrej commented Oct 21, 2024

No description provided.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is quite repetitive and I was considering some kind of matrix approach, but ultimately didn't do it as it seems to me the test cases are so short and concise, the maintainability might actually suffer if we try to DRY it. Let me know what you think 🙏

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't mind either way. You could also reuse the same domActions instance across multiple tests, then the tests would be basically one-liners.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uuu nice, I quite like that! The one-liners are pretty elegant 🚀 👏

@muodov muodov self-requested a review October 22, 2024 08:15
@freiondrej freiondrej force-pushed the dom-actions-wtr-element-visible branch from 1d398f7 to 9b1d328 Compare October 22, 2024 18:28
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't mind either way. You could also reuse the same domActions instance across multiple tests, then the tests would be basically one-liners.

});
</script>
<div id="none-visible">
<button style="display:none">This one is invisible</button>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also add a case when a button element is not hidden itself, but its parent is?

<div style="display:none">
  <button>I am hidden too</button>
</div>

There are many more possible cases, but these are probably most common.

Copy link
Contributor Author

@freiondrej freiondrej Oct 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean add it as simply as this 23b3368?

@freiondrej freiondrej requested a review from muodov October 23, 2024 19:04
@muodov muodov added the tests Add or improve existing tests label Oct 24, 2024
Copy link
Member

@muodov muodov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! thanks again!

@muodov muodov merged commit c22c1c3 into duckduckgo:main Oct 24, 2024
5 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests Add or improve existing tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants