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

Ensure testing elements are properly reset/cleared. #226

Merged
merged 1 commit into from
Oct 17, 2017

Conversation

rwjblue
Copy link
Member

@rwjblue rwjblue commented Oct 17, 2017

Prior to this change, the innerHTML of the #ember-testing element was reset after each test (much in the same way as QUnit itself resets the #qunit-fixture between tests). There were two specific issues with the approach:

  1. The teardownRenderingContext was required to run after the teardownContext. If the order was set correctly (teardownRenderContext(this) then teardownContext(this)) errors would be thrown because Ember is attempting to clean up the rendering engine and its expected DOM elements to remove are no longer present. This meant that we could not use the normal rule of thumb for these things: that teardown is done in the reverse order of setup.
  2. Any attributes or properties added to the #ember-testing element were not being cleaned up, and caused a large number of cascading test failures. Ember's acceptance tests set an ember-application class on what it thinks is the rootElement, if it sees that class is already present it throws an error. Due to the way tests were being improperly cleaned up, one failed acceptance test (which failed in such a way as proper cleanup could not happen) would essentially cause all of the remaining acceptance tests to fail.

The fix here was to swap from capturing and resetting the #ember-testing elements innerHTML to capturing and resetting the #ember-testing-container's innerHTML. This has the effect of ensuring that the #ember-testing element (which is contained within the #ember-testing-container element) is reset completely, and allows Ember's rendering system to continue to do its normal cleanup without error (because the elements it expected to exist are still present).

Prior to this change, the `innerHTML` of the `#ember-testing` element was reset
after each test (much in the same way as QUnit itself resets the `qunit-fixture`
between tests). There were two specific issues with the approach:

1. The `teardownRenderingContext` was **required** to run after the
  `teardownContext`. If the order was set correctly (`teardownRenderContext(this)`
  _then_ `teardownContext(this)`) errors would be thrown because Ember is
  attempting to clean up the rendering engine and its expected DOM elements to
  remove are no longer present. This meant that we could not use the normal rule
  of thumb for these things: that teardown is done in the reverse order of setup.
2. Any attributes or properties added to the `#ember-testing` element were
  **not** being cleaned up, and caused a large number of cascading test
  failures. Ember's acceptance tests set an `ember-application` class on what it
  thinks is the `rootElement`, if it sees that class is already present it
  throws an error. Due to the way tests were being improperly cleaned up, one
  failed acceptance test (which failed in such a way as proper cleanup could not
  happen) would essentially cause **all** of the remaining acceptance tests to
  fail.

The fix here was to swap from capturing and resetting the `#ember-testing`
elements `innerHTML` to capturing and resetting the `#ember-testing-container`'s `innerHTML`.
This has the effect of ensuring that the `#ember-testing` element (which is
contained _within_ the `#ember-testing-container` element) is reset completely,
and allows Ember's rendering system to continue to do its normal cleanup without
error (because the elements it expected to exist are still present).
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.

2 participants