From 7ad93d2c5331279b798055b46034c92da031f1e7 Mon Sep 17 00:00:00 2001 From: Robert Jackson Date: Tue, 17 Oct 2017 11:46:20 -0400 Subject: [PATCH] Ensure testing elements are properly reset/cleared. 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). --- addon-test-support/setup-rendering-context.js | 7 ++- tests/unit/settled-test.js | 2 +- tests/unit/setup-rendering-context-test.js | 2 +- tests/unit/teardown-rendering-context-test.js | 53 +++++++++++++++++++ 4 files changed, 58 insertions(+), 6 deletions(-) create mode 100644 tests/unit/teardown-rendering-context-test.js diff --git a/addon-test-support/setup-rendering-context.js b/addon-test-support/setup-rendering-context.js index bcd8de0ac..2cdf0fbf5 100644 --- a/addon-test-support/setup-rendering-context.js +++ b/addon-test-support/setup-rendering-context.js @@ -39,13 +39,12 @@ export function clearRender() { export default function(context) { let guid = guidFor(context); - let rootTestElement = document.getElementById('ember-testing'); - let fixtureResetValue = rootTestElement.innerHTML; + let testElementContainer = document.getElementById('ember-testing-container'); + let fixtureResetValue = testElementContainer.outerHTML; RENDERING_CLEANUP[guid] = [ () => { - rootTestElement.innerHTML = fixtureResetValue; - element = undefined; + testElementContainer.innerHTML = fixtureResetValue; }, ]; diff --git a/tests/unit/settled-test.js b/tests/unit/settled-test.js index 2141d6b5e..3d348a5a1 100644 --- a/tests/unit/settled-test.js +++ b/tests/unit/settled-test.js @@ -185,8 +185,8 @@ module('settle', function(hooks) { this.server.shutdown(); await settled(); - teardownContext(this); teardownRenderingContext(this); + teardownContext(this); }); test('it works when async exists in `init`', async function(assert) { diff --git a/tests/unit/setup-rendering-context-test.js b/tests/unit/setup-rendering-context-test.js index 0906a48f0..d2a206e7d 100644 --- a/tests/unit/setup-rendering-context-test.js +++ b/tests/unit/setup-rendering-context-test.js @@ -39,8 +39,8 @@ module('setupRenderingContext', function(hooks) { }); hooks.afterEach(function() { - teardownContext(this); teardownRenderingContext(this); + teardownContext(this); }); test('render exposes an `.element` property', async function(assert) { diff --git a/tests/unit/teardown-rendering-context-test.js b/tests/unit/teardown-rendering-context-test.js new file mode 100644 index 000000000..d7b27c8b8 --- /dev/null +++ b/tests/unit/teardown-rendering-context-test.js @@ -0,0 +1,53 @@ +import { module, test } from 'qunit'; +import { + setupContext, + setupRenderingContext, + teardownContext, + teardownRenderingContext, +} from 'ember-test-helpers'; +import hasEmberVersion from 'ember-test-helpers/has-ember-version'; + +module('setupRenderingContext', function(hooks) { + if (!hasEmberVersion(2, 4)) { + return; + } + + hooks.beforeEach(function() { + setupContext(this); + setupRenderingContext(this); + }); + + hooks.afterEach(function() { + teardownContext(this); + }); + + test('clears any attributes added to the ember-testing div', function(assert) { + let beforeTeardownEl = document.getElementById('ember-testing'); + beforeTeardownEl.setAttribute('data-was-set', ''); + + assert.ok( + beforeTeardownEl.hasAttribute('data-was-set'), + 'precond - attribute is present before teardown' + ); + assert.ok(document.contains(beforeTeardownEl), 'precond - ember-testing element is in DOM'); + + teardownRenderingContext(this); + + let afterTeardownEl = document.getElementById('ember-testing'); + + assert.notOk( + afterTeardownEl.hasAttribute('data-was-set'), + 'attribute is not present on ember-testing that is in DOM' + ); + assert.ok(document.contains(afterTeardownEl), 'ember-testing element is still in DOM'); + + assert.ok( + beforeTeardownEl.hasAttribute('data-was-set'), + 'attribute is still present on prior ember-testing element after teardown' + ); + assert.notOk( + document.contains(beforeTeardownEl), + 'previous ember-testing element is no longer in DOM' + ); + }); +});