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

Tests: Sandbox document.body.innerHTML in ExporterTest #19979

Merged
merged 3 commits into from Jan 12, 2018

Conversation

islemaster
Copy link
Contributor

While working on some new tests I discovered that these ExporterTests were leaving a #divApplab element in the DOM, which interfered with my new tests. This PR adds a little utility to save and restore the document body around a test, and applies it to this set of tests which will unblock my other PR.

I'm gradually working through our test suite finding other places this should be applied, but that's a larger process and will take a while. I'd like to get this fix in on its own for now.

Worked out, using an afterEach step in unit-tests.js, that these tests (the ones that use the `runExportedApp` helper) were leaving content in the phantomjs DOM - in particular, that they were leaving a `#divApplab` lying around, which messed with another test I was trying to write.

Just saving and restoring document.body.innerHTML so that these tests don't leak DOM elements.
@@ -342,6 +342,12 @@ export function restoreOnWindow(key) {
delete originalWindowValues[key];
}

export function sandboxDocumentBody() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Worth a comment explaining what it does, why it does it, and what sorts of drawbacks in might have (i.e. the fact that this will blow away attached event handlers).

@islemaster islemaster merged commit 5cb74a6 into staging Jan 12, 2018
@islemaster islemaster deleted the unit-test-cleanup branch January 12, 2018 23:33
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

2 participants