Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 1 addition & 23 deletions addon/src/setup-context.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import type { Resolver } from '@ember/owner';
import { setOwner } from '@ember/application';

import buildOwner, { type Owner } from './build-owner.ts';
import { _setupAJAXHooks, _teardownAJAXHooks } from './settled.ts';
import { _setupAJAXHooks } from './settled.ts';
import { _prepareOnerror } from './setup-onerror.ts';
import Ember from 'ember';
import {
Expand All @@ -19,10 +19,6 @@ import global from './global.ts';
import { getResolver } from './resolver.ts';
import { getApplication } from './application.ts';
import getTestMetadata from './test-metadata.ts';
import {
registerDestructor,
associateDestroyableChild,
} from '@ember/destroyable';
import {
getDeprecationsForContext,
getDeprecationsDuringCallbackForContext,
Expand Down Expand Up @@ -211,20 +207,6 @@ export function resumeTest(): void {
context.resumeTest();
}

/**
@private
@param {Object} context the test context being cleaned up
*/
// eslint-disable-next-line @typescript-eslint/no-unused-vars
function cleanup(context: BaseContext) {
_teardownAJAXHooks();

// SAFETY: this is intimate API *designed* for us to override.
(Ember as any).testing = false;

unsetContext();
}

/**
* Returns deprecations which have occurred so far for a the current test context
*
Expand Down Expand Up @@ -410,8 +392,6 @@ export default function setupContext<T extends object>(

_backburner.DEBUG = true;

registerDestructor(context, cleanup);

_prepareOnerror(context);

return Promise.resolve()
Expand All @@ -438,8 +418,6 @@ export default function setupContext<T extends object>(
return buildOwner(getApplication(), getResolver());
})
.then((owner) => {
associateDestroyableChild(context, owner);

Object.defineProperty(context, 'owner', {
configurable: true,
enumerable: true,
Expand Down
13 changes: 11 additions & 2 deletions addon/src/teardown-context.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import type { TestContext } from './setup-context';
import settled from './settled.ts';
import Ember from 'ember';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

import { unsetContext } from './setup-context.ts';
import settled, { _teardownAJAXHooks } from './settled.ts';
import { _cleanupOnerror } from './setup-onerror.ts';
import { destroy } from '@ember/destroyable';

Expand Down Expand Up @@ -30,7 +32,14 @@ export default function teardownContext(
.then(() => {
_cleanupOnerror(context);

destroy(context);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since this wasn't here before, can we remove it 😅

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


unsetContext();

destroy(context.owner);
})
.finally(() => {
if (waitForSettled) {
Expand Down
10 changes: 0 additions & 10 deletions test-app/tests/unit/dom/fill-in-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,16 +16,6 @@ if (isFirefox || isChrome) {
clickSteps.push('selectionchange');
}

/**
* Prior to Chrome 129 (canary),
* Chrome 127.x emits an extra selectionchange event sometimes
*
* Delete this once we don't want to test against Chrome 127
*/
if (isChrome) {
clickSteps.push('selectionchange');
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


module('DOM Helper: fillIn', function (hooks) {
if (!hasEmberVersion(2, 4)) {
return;
Expand Down
13 changes: 13 additions & 0 deletions test-app/tests/unit/setup-application-context-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -257,4 +257,17 @@ module('setupApplicationContext', function (hooks) {
'after click resolved',
]);
});

test('can reset context during a test', async function (assert) {
await visit('/');
assert.equal(currentURL(), '/');

await teardownContext(this);
await setupContext(this);
await setupApplicationContext(this);
assert.equal(currentURL(), null);

await visit('/');
assert.equal(currentURL(), '/');
});
});
14 changes: 14 additions & 0 deletions test-app/tests/unit/setup-context-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -943,5 +943,19 @@ module('setupContext', function (hooks) {
}
}
});

test('can reset context during a test', async function (assert) {
assert.expect(1);

let context = await setupContext({});
try {
await teardownContext(context);
context = await setupContext(context);
} finally {
await teardownContext(context);
}

assert.ok(true, 'No error calling setupContext after teardownContext');
});
});
});
20 changes: 4 additions & 16 deletions test-app/tests/unit/teardown-context-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ import hasjQuery from '../helpers/has-jquery';
import ajax from '../helpers/ajax';
import Pretender from 'pretender';
import setupManualTestWaiter from '../helpers/manual-test-waiter';
import { registerDestructor } from '@ember/destroyable';

module('teardownContext', function (hooks) {
if (!hasEmberVersion(2, 4)) {
Expand Down Expand Up @@ -65,28 +64,17 @@ module('teardownContext', function (hooks) {
assert.strictEqual(getContext(), undefined, 'context is unset');
});

test('destroyables registered with the context are invoked', async function (assert) {
registerDestructor(context, () => {
assert.step('destructor was ran');
});

assert.step('teardown started');

test('the owner is destroyed', async function (assert) {
await teardownContext(context);

assert.step('teardown completed');

assert.verifySteps([
'teardown started',
'destructor was ran',
'teardown completed',
]);
assert.ok(context.owner.isDestroyed);
});

test('the owner is destroyed', async function (assert) {
test('the context is not destroyed', async function (assert) {
await teardownContext(context);

assert.ok(context.owner.isDestroyed);
assert.notOk(context.isDestroyed);
});

test('the application instance is destroyed and unwatched', async function (assert) {
Expand Down