Leaking states between tests (because of acceptance tests)

[alexander-alvarez]

Using a mix of nested-module-testing for integration and unit tests, and moduleForAcceptance for acceptance tests:

---

If tests modify the testing container but don't call teardownRenderingContext then the state is carried throughout the tests because of
https://github.com/emberjs/ember-test-helpers/blob/master/addon-test-support/%40ember/test-helpers/setup-rendering-context.js#L43

I just got bit by an acceptance tests that runs an initializer that modifies the root element https://github.com/yapplabs/ember-modal-dialog/blob/master/addon/initializers/add-modals-container.js#L31

and renderingTests that run after it (and use find('*')) break because the first element is modal-overlays instead of the component under test

Ideas on how to remediate this:

• Don't carry over the state? (was it there for a reason?)
• Document that we may have to handle these things in your acceptance tests after hooks, (until we overhaul moduleForAcceptance)
• ?

[rwjblue]

Some somewhat random thoughts:

• By default in ember-cli@2.17+ (see Update to ember-cli-qunit@4.1.1. ember-cli/ember-cli#7437) will actually run initializers before all tests (integration or not) which would at least make this consistent (e.g. not order dependent).

• When this library adds acceptance testing support, it will properly cleanup the fixture state after each test (just like we do for both setupRenderingTest and moduleForComponent already).

• I would generally suggest that folks avoid find('*') I'm not really sure what value it provides? Generally speaking, your tests should use a selector that is somehow related to the thing being tested (IOW relying on the implicit first element seems super brittle).

• It would be possible to modify ember-modal-dialog to setup and teardown per-test, but the current logic in the initializer is seems fine to me (IOW there is no reason to do this per-test IMHO since there is shared instance state)?

[alexander-alvarez]

I would generally suggest that folks avoid find('*') I'm not really sure what value it provides? Generally speaking, your tests should use a selector that is somehow related to the thing being tested (IOW relying on the implicit first element seems super brittle).

IMO find('*').textContent.trim() is the equivalent of this.$().trim()

I've also used it for this:
assert.notOk(find('*').children.length, 'is a tagless component, and does not affect the DOM');

For some reason (if you want I can try to find out) I wasn't getting sporadic test failures with the same tests using the old moduleFor* syntax...

Where does that leave us / what's the recommendation?

EDIT
I only had a handful of acceptance test files, so for my purposes I just added find('#modal-overlays').remove() in the afterEach

[rwjblue]

IMO find('*').textContent.trim() is the equivalent of this.$().trim()

Hmm, its definitely not the same at all. this.$().trim() will give you the complete text contents of the current testing element, whereas find('*').textContent is only the text content of the first element. Why not just use this.element.textContent.trim() in this scenario? The thing you are attempting to assert is that the text is empty (or whatever its value is), in this case it has nothing to do with what is returned as *.

For some reason (if you want I can try to find out) I wasn't getting sporadic test failures with the same tests using the old moduleFor* syntax...

Ya, it is worthwhile to figure this out IMHO. My statement before was based on the fact that moduleForAcceptance does not do anything to clean up the applications rootElement and therefore I would expect that after acceptance tests run (even before the newer nested module work here) we would already have had contents in the #ember-testing element.

I only had a handful of acceptance test files, so for my purposes I just added find('#modal-overlays').remove() in the afterEach

If you do need to do this, I would do it in a QUnit.testDone instead of having to remember this in each acceptance test...

Where does that leave us / what's the recommendation?

Not sure. The biggest issue is that we just need to land the acceptance testing system here ASAP, but for that to work we need to settle the RFC first...

[alexander-alvarez]

Hmm, its definitely not the same at all. this.$().trim() will give you the complete text contents of the current testing element, whereas find('*').textContent is only the text content of the first element. Why not just use this.element.textContent.trim() in this scenario? The thing you are attempting to assert is that the text is empty (or whatever its value is), in this case it has nothing to do with what is returned as *.

Then I was mistaken... Probably because I didn't know this.element was a thing in the test... lol.

Another update...
I updated my helpers/destroy-app.js to include find('#modal-overlays').remove();
since it turns out I'm booting full fledged apps in some "unit" and "integration" and destroyApp is used in module-for-acceptance test, I found this to be the best place for for the time being