Ensure ember-testing-container is reset properly. #229
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The changes made in 7ad93d2 introduced a bug in the way that the testing DOM is cleaned up between tests. The error was quite simple: we were storing the `storedValue = container.outerHTML` before starting the test then resetting `container.innerHTML = storedValue`. This created a DOM node leak for each test processed. This fixes the issue for both the new testing API's in 0.7 and the older pre-existing system. It also adds some post-test checks to ensure that the DOM is indeed what we expect after each test is completed.
Turbo87
approved these changes
Oct 20, 2017
|
Why exactly do we need two divs? |
|
We have had them historically (I believe the container is used for styling the preview window stuff), this library just ignored the fact that we had two which allows this little “trick” (using innerHTML of the container div to ensure the attributes of the ember-testing div are fully reset). FWIW, in qunit land (not sure how mocha does this) we should probably just embed the container inside the qunit-fixture div and all of this cleanup would be taken care of for us. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The changes made in 7ad93d2 introduced a bug in the way that the testing DOM is cleaned up between tests. The error was quite simple: we were storing the
storedValue = container.outerHTMLbefore starting the test then resettingcontainer.innerHTML = storedValue. This created a DOM node leak for each test processed.This fixes the issue for both the new testing API's in 0.7 and the older pre-existing system. It also adds some post-test checks to ensure that the DOM is indeed what we expect after each test is completed.
Fixes #228.