[WIP] fixes for ember-source@3.13

[ghost]

See the discussion in #431. I think this may be the underlying issue in #430 and blocking TryGhost/Admin#1335.

Without the changes to setup-test.js the first test I added fails. I observed this in my application as well. It appeared that properties set on this in a beforeEach no longer persisted between tests.
Upon removing the caching behavior in setup-test.js the first test passes as I expect. I would like to understand why this is the case though.

• Why do we copy test context properties using assign or merge? Or what purpose did that logic serve when introduced?

The second test I added fails with a helpful message about name being a tracked property. I would expect this to fail so should I assert that it does fail?

[ghost]

I ran this on our integration tests and found it completes running the tests. 70 tests fail that did not in prior versions of ember but that makes me think the assertions are off or something has changed to warrant the tests failing. Without this change integration tests fail on unhandled errors within ember-mocha itself and the browser reloads at about halfway through the suite.

[Turbo87]

hmm... I assume that it was originally implemented like that for a reason, and I'm a little worried that we're accidentally breaking things with this PR 🤔

[ghost]

@Turbo87 I feel the same way :). I stumbled upon this approach by looking at what ember-qunit does for setupRenderingTest. So I think a better understanding of ember test helpers and the proper way to clear test context state in the 3.13 series would help determine the best fix.

[ghost]

From exploring the blame it appears the logic I removed in setup-test was introduced in #190.

[Turbo87]

@simonihmig do you have time to take a look at this? 🙏

[simonihmig]

I am afraid at this point in time I am of limited use here. But I recently started to upgrade an Ember with a large ember-mocha test suite to 3.13 (with a bunch of failing tests, but so far different issues). So I might eventually run into the same issues maybe...

Why do we copy test context properties using assign or merge? Or what purpose did that logic serve when introduced?

1. We don't really copy/cache anything there. Rather it cleans up any state that has been set on the this context between the beforeEach and afterEach hooks. So the intend is to prevent state to leak between tests AFAICT.
2. IIRC when starting on work to support the (at that time) "new" testing APIs, I got a code snippet from @rwjblue that contained that logic...

Without the changes to setup-test.js the first test I added fails

What's the error message? And that happens only for Ember 3.13 if I got you right?

[ghost]

@simonihmig thank you for the added insight; points 1 and 2 make sense.

Correct; we only see this when updating to 3.13.x (we are on 3.12). The specific failure in my test suite was Error: Property set failed: object in path "some_property" could not be found. The test was setting some_property to the this context in a beforeEach hook. This in turn cascaded failures through the test suite. I was unable to reproduce this in a separate clean app and so turned to reproducing it here in the setupRenderingTest.

[leepfrog]

Thanks for the investigation into this @efx, for what it's worth, this patch has resolved my issue on 3.14-beta.2.

[simonihmig]

So my findings with this so far, after trying to upgrade our ember-mocha tested app to 3.13:

• I can confirm the issue itself: given a previous test has set foo on its this context, this happens in a following test:

this.set('foo', 'bar');
this.get('foo'); // undefined

• when running the test in isolation (i.e. no other test has "polluted" the this context), things work as expected

• When calling this.set(), this delegates to Ember.set() which you can see here:
  

  MANDATORY_SETTERS is a WeakMap, and obj is our test's this context - which is shared among tests in Mocha - so setters contains some created from a previous test. So if the same key was used, we get a setter there that was created from a previous test. Which seems to be the root issue. And probably explains why this issue is related to the new Ember version.

• when running the same test in isolation, the code path is different (as setters is undefined), as you can see here:
  

• I tried the changes in this PR. And while they do make my test pass as well, they don't seem to be the right solution yet unfortunately! The reason is that now the this context is not correctly cleaned up after tests, which was the reason for the code in the afterEach hook as mentioned in my previous comment. So when you set foo in a previous test, you will see the same foo property still being present in any following test, i.e. test state is leaking! Which can cause unwanted side effects.

• I am not aware about QUnit internals, but I would guess this is probably not shared among tests, so this is not a problem there?

I don't have an idea yet how this should be solved properly. 😔

@Turbo87 @rwjblue any idea?

[ghost]

@simonihmig thank you for the thorough investigation!

Which seems to be the root issue. And probably explains why this issue is related to the new Ember version.

This is a helpful summary. I did some bisecting in this comment and I think emberjs/ember.js@f046bbe introduced some of the related logic.

I tried the changes in this PR. And while they do make my test pass as well, they don't seem to be the right solution yet unfortunately! The reason is that now the this context is not correctly cleaned up after tests,

This confirms my suspicion that while my PR removes logic showing the bug it indeed is not the correct fix. So it seems we need to implement test set up and tear down that accounts for the changed semantics of ember setters / getters in 3.13.x.

[ghost]

I tried a simpler approach of the existing logic: https://github.com/emberjs/ember-mocha/compare/master...efx:fix-430-1?expand=1. These changes should preserve the garbage collection between tests. It works reliably for setupTest but I can still reproduce this.set delegating to a stale, user set test context property in setupRenderingTest. So I am going to keep using that branch to find the fix for all scenarios.

[ghost]

I am not aware about QUnit internals, but I would guess this is probably not shared among tests, so this is not a problem there?

Without digging into the internals it does look like QUnit does not reuse properties on this.

I have linked ember-source locally and given a quick analog in ember-qunit it appears that the setupMandatorySetter is not called in ember-qunit but is for in ember-mocha when executing the combination of this.set described above.