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

Add tests for DomActions.prehide() #552

Merged
merged 1 commit into from
Jan 10, 2025

Conversation

freiondrej
Copy link
Contributor

@freiondrej freiondrej commented Nov 29, 2024

As described in the mini README, I had to split the tests into multiple files, as I didn't find any option of WTR to refresh files between runs.

I suppose potentially we could introduce such logic ourselves by using the "barebones" variant of html tests (the 2nd code snippet in this page), where runTests would be replaced by direct calls to specific test cases and we would perform a reload after each of them, but I'm not sure it's worth it, let me know if I should give it a try 🚀

EDIT: Or, we could leverage undoPrehide() to get rid of the element again. However, I'm not sure if we want to couple the tests to its implementation like that.

@freiondrej freiondrej force-pushed the dom-actions-wtr-prehide branch from 8c767e4 to c72900e Compare November 29, 2024 10:15
@sammacbeth sammacbeth requested a review from muodov December 6, 2024 10:34
domActions = instantiateDomActions();
});

it('returns false when selector is empty string', () => {
Copy link
Member

Choose a reason for hiding this comment

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

interesting, empty selector was never supposed to be passed here I think. In fact, right now it produces an invalid CSS in the style tag.
Invalid CSS is ignored by the browser, and we don't pass empty selectors anyways, so it's not a big deal. But we should clean it up some day, and handle empty selectors in this method.

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 @freiondrej!

@muodov muodov added the tests Add or improve existing tests label Jan 10, 2025
@muodov muodov merged commit 5b6a898 into duckduckgo:main Jan 10, 2025
4 of 6 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