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

[html] Clean state between dialog positioning tests #19657

Merged
merged 2 commits into from Oct 13, 2019

Conversation

dpogue
Copy link
Contributor

@dpogue dpogue commented Oct 13, 2019

Calling reset() at the end of every test to close the modal dialog will only be invoked if the test passes. If an expectation fails, the call to reset() will never run and the dialog will remain modally open on the page. This causes all further tests to fail because calling showModal() on the dialog while it is open is an error.

Instead, set up reset() to be called as a cleanup after every test regardless of whether the test passes or fails.

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

Could be cleaner by just doing this.add_cleanup(reset), but LGTM. Thanks for fixing!

@dpogue
Copy link
Contributor Author

dpogue commented Oct 13, 2019

Good point! I'll make that change :)

Calling `reset()` at the end of every test to close the modal dialog
will only be invoked if the test passes. If an expectation fails, the
call to `reset()` will never run and the dialog will remain modally open
on the page. This causes all further tests to fail because calling
`showModal()` on the dialog while it is open is an error.

Instead, set up `reset()` to be called as a cleanup after every test
regardless of whether the test passes or fails.
The `add_cleanup()` calls need to be registered outside of the
`assert_throws` block to ensure that the dialogs are actually closed.
@domenic domenic merged commit 79c3825 into web-platform-tests:master Oct 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants