Use a disposable object to serve as context for Ember test helpers

[kobsy]

Summary

Because Ember 3.13 introduces an inaccessible WeakMap for framework-provided setters (to support tracked properties) keyed off of the owning object, the contexts used in Ember tests need to be disposable objects, lest a stale setter be used for a property sharing the same name with one from a previous test. To balance the ergonomics of Mocha, this change introduces such a disposable object, which gets passed to the various @ember/test-helper-provided setup methods, and then forwarding attributes are set up and torn down on this that provide transparent access to the disposable context.

One drawback to this approach is that attributes added to the Ember test context without invoking setUnknownProperty() will be unavailable on the Mocha context unless an empty value is added to the Ember context before setup; there is already an example of this in the application test function visit(), which sets element on the context without it having been added to the context as part of setupApplicationContext(). This solution would necessitate such attributes be added to the disposable context object before forwarding is set up (as is done already with element). This is somewhat mitigated by adding unknownProperty() and setUnknownProperty() to the EmberTestContext class, as it allows templates to access attributes which may have been set on the Mocha context without using this.set, but it's still not ideal in that the calling getter/setter must respect the unknownProperty()/setUnknownProperty() methods, which a native getter/setter will not.

This PR would be an alternate solution from #460 to address #430 (and indeed two of the added tests were pulled almost directly from #460 and #450; the other two represent what is perhaps not best practices, but they validate that it continues to work how it did previously, at any rate). I'd done some digging on my own, but I was glad to have my suspicions confirmed by @simonihmig in the discussion on #450. Thanks! 🙂

[ghost]

should this only apply to rendering tests?

[kobsy]

That had been my initial thought, but the visit() method, used in acceptance tests, assigns context.element without it having been set up on the context beforehand. I suppose we could assign element to the context in setupApplicationTest() (it already gets assigned as part of the call to setupRenderingContext() in setupRenderingTest()) instead of declaring it here, so that it's more explicit why it's needed. 🤔

[ghost]

I like how explicit this approach is! I have been stumbling my way throughout each piece here.
I'm curious how the logic here overlaps with ember-test-helpers. It seems like we would want to avoid maintaining a version of testing context here that could diverge from ember-test-helpers.

[ghost]

And thank you @kobsy for taking this on!

FWIW I just ran this on our application's codebase and am seeing a number of errors relating to the rendering test context not set up properly.

[kobsy]

I think you have a valid point about this approach both being very explicit ... but also about it adding a lot of surface area that would need to be maintained. 😞 I'd be curious what sorts of errors you're encountering; I was able to incorporate this with our application's tests and was able to run without any errors, but I may not have taken the same approaches as you have in your tests to setting up the context. (I fear besides maintenance, this kind of mechanism is pretty brittle in-and-of itself, with plenty of edge cases and "gotchas" waiting to be encountered.)

With that said, I did some more digging and found an example how I might access Ember's internal method to clean up the offending WeakMap entries. I carried over the tests and then implemented the solution in #462. While I don't love using private API in this way, the solution is so much less complex than what's presented in this PR, I think I'd prefer the other solution over this one. 😅

[ghost]

I'd be curious what sorts of errors you're encountering;

Our integration test suite easily has 2,000 or more tests. Quick survey:

• assertions on DOM state failing because the properties were not persisting to the component
• TypeError: Cannot read property 'call' of undefined
• Error: Must setup rendering context before attempting to interact with elements.

After these two there are a cascade of errors I've seen before.

• Error: Assertion Failed: You cannot use the same root element (DIV) multiple times in an Ember.Application
• TypeError: Cannot use 'in' operator to search for 'destroy' in undefined
  • This one happens because of our cleanup logic doesn't check to see if this is defined.

With that said, I did some more digging and found an example how I might access Ember's internal method to clean up the offending WeakMap entries

Thanks to your explanations in your PR and all the pieces of the conversation I've been following, I started trying to do the same locally in #460 :)

While I don't love using private API in this way, the solution is so much less complex than what's presented in this PR, I think I'd prefer the other solution over this one. 😅

Totally agree.

[kobsy]

I think in light of the comments around this solution, I'm going to go ahead and close this PR. It was a good spike on what it would take to supply a disposable context object, but because of the complexity involved, it just doesn't seem like a good direction for solving this problem.