Make test context reusable during a test #1511
Conversation
In v2.0.0 @ember/test-helpers refactored to use @ember/destroyable for teardown which also meant that the application context was destroyed during teardown, where previously it was only cleaned up and the application was destroyed. In our application tests we simulate a page refresh by calling `teardownContext`` and `setupContext`. The destroyable change broke this functionality because the context is now marked as destroyed. This commit effectively reverts the change to use @ember/destroyable and adds tests to ensure that the context is not destroyed so that it can be reset during a test.
| @@ -1,5 +1,7 @@ | |||
| import type { TestContext } from './setup-context'; | |||
| import settled from './settled.ts'; | |||
| import Ember from 'ember'; | |||
There was a problem hiding this comment.
we cannot import from ember as it's planned to be deprecated soonish in the v6 series.
See: emberjs/rfcs#1003
the RFC also has a big list of alternate ways to get what you need though.
There was a problem hiding this comment.
Good to know. This isn't new though, just moved from setup-context.ts and there are several other imports currently in the repo.
Also this project is setting Ember.testing, I only see an alternative for checking it.
|
first, hello!!!! second, wouldn't we want "refresh" to mean you get a whole new context? a destroy + new one? (as close to actual page refresh as possible?) |
Hello, yourself :) I don't think we do want a new context. We only want a new app (context.owner). The context is the qunit context passed in from ember-qunit and has things like |
|
that makes sense! |
| */ | ||
| if (isChrome) { | ||
| clickSteps.push('selectionchange'); | ||
| } |
There was a problem hiding this comment.
This is unrelated to the context reuse issue but fixes test failures currently on master. Since tests are running on Chrome 130 now, I assume this is safe to remove.
| @@ -30,7 +32,14 @@ export default function teardownContext( | |||
| .then(() => { | |||
| _cleanupOnerror(context); | |||
|
|
|||
| destroy(context); | |||
There was a problem hiding this comment.
I think the only potential impact of this change would be that if consumers are calling registerDestructor on the context for their own cleanup, then that would break and they would need to call it on context.owner instead.
| _teardownAJAXHooks(); | ||
|
|
||
| // SAFETY: this is intimate API *designed* for us to override. | ||
| (Ember as any).testing = false; |
There was a problem hiding this comment.
since this wasn't here before, can we remove it 😅
There was a problem hiding this comment.
oh apologies, you said it was from here: https://github.com/emberjs/ember-test-helpers/pull/1511/files#diff-93af636d6cd5ada55ea13d76db06d4e9447f451caae3fd2dc83a7cd4783bd385L223
ok. nevermind.
This needs some thinking unrelated to this PR
|
Any updates/concerns on merging this? |
In v2.0.0 @ember/test-helpers refactored to use
@ember/destroyablefor teardown which also meantthat the application context was destroyed during teardown, where previously it was only cleaned up
and the application (
context.owner) was destroyed.In our application tests we simulate a page refresh by calling
teardownContextandsetupContext. The destroyable change broke this functionality because the context is now markedas destroyed.
This commit effectively reverts the change to use
@ember/destroyableand adds tests to ensure thatthe context is not destroyed so that it can be reset during a test.
This addresses #1452 raised by @mixonic
The @ember/destroyable change was made in #914