Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Upgrade Ember Data to version 4.12.4 #974

Merged
merged 8 commits into from
Nov 16, 2023
Merged

Upgrade Ember Data to version 4.12.4 #974

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
062ca2e
Upgrade `ember-data` to `4.12.4`
VasylMarchuk Oct 8, 2023
4a39b91
Add `AbortController` to FastBoot's `buildSandboxGlobals`
VasylMarchuk Nov 9, 2023
2d4404e
Fix some failing tests by adding an extra `await settled()` call
VasylMarchuk Nov 11, 2023
002dcc3
Fix some failing tests counting API requests incorrectly
VasylMarchuk Nov 11, 2023
42e7ef6
Fix some failing tests by adding an extra `await animationsSettled()`…
VasylMarchuk Nov 11, 2023
b5bdfdb
Speed up concepts tests by adding `setupAnimationTest` call
VasylMarchuk Nov 13, 2023
2ba3531
Add a comment explaining what `buildSandboxGlobals` is
VasylMarchuk Nov 16, 2023
a0858f5
Add comments to investigate extra `await settled()` calls
VasylMarchuk Nov 16, 2023
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
9 changes: 9 additions & 0 deletions config/fastboot.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
module.exports = function (/* environment */) {
return {
buildSandboxGlobals(defaultGlobals) {
return Object.assign({}, defaultGlobals, {
AbortController,
Copy link
Member

Choose a reason for hiding this comment

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

@VasylMarchuk I remember that you linked to some github issue that mentioned this - can we add a comment linking to that here please?

Copy link
Collaborator Author

@VasylMarchuk VasylMarchuk Nov 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I found a reference to a similar problem in this discussion: https://discuss.emberjs.com/t/abortcontroller-errors-after-update/20259/4

It links to an issue in the FastBoot repo: ember-fastboot/ember-cli-fastboot#913

There is a vague reference to this buildSandboxGlobals in the FastBoot readme as well: https://github.com/ember-fastboot/ember-cli-fastboot#fastboot-configuration

Basically, some of the newer global types/methods (fetch, AbortController, ReadableStream etc) have to be explicitly defined in this list of "globals" that FastBoot makes available in the "sandbox", even though they are natively present in node itself long ago. And it's not really clear and nowhere mentioned in the docs about what's already in this sandbox and what has to be passed explicitly.

Copy link
Member

Choose a reason for hiding this comment

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

@VasylMarchuk I meant to add a comment in the code 🙂 Just for future reference, in case someone wonders what this does

});
},
};
};
3,052 changes: 2,158 additions & 894 deletions package-lock.json
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@
"ember-composable-helpers": "^5.0.0",
"ember-concurrency": "^3.1.1",
"ember-cookies": "^1.0.0",
"ember-data": "~4.11.3",
"ember-data": "~4.12.4",
"ember-exam": "^8.0.0",
"ember-fetch": "^8.1.2",
"ember-in-viewport": "^4.1.0",
Expand Down
3 changes: 3 additions & 0 deletions tests/acceptance/concept-admin/edit-blocks-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { drag } from 'ember-sortable/test-support';
import { module, test } from 'qunit';
import { setupApplicationTest } from 'ember-qunit';
import { setupMirage } from 'ember-cli-mirage/test-support';
import { settled } from '@ember/test-helpers';
import { signInAsStaff } from 'codecrafters-frontend/tests/support/authentication-helpers';
import blocksPage from 'codecrafters-frontend/tests/pages/concept-admin/blocks-page';
import testScenario from 'codecrafters-frontend/mirage/scenarios/test';
Expand Down Expand Up @@ -61,6 +62,8 @@ module('Acceptance | concept-admin | edit-blocks', function (hooks) {
await blocksPage.editableBlocks[1].clickOnSaveButton();

await blocksPage.clickOnPublishChangesButton();
await settled();
Copy link
Member

Choose a reason for hiding this comment

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

This is interesting - clickOnPublishChangesButton internally uses clickable() from ember-cli-page-object, and that should be calling settled() automatically.

This can be problematic, it'll mean that we have to keep adding settled() in random places without exactly knowing what we're waiting on... I did try this locally, and it looks like when settled() isn't present, blocksWithMetadata (the property that renders editableBlocks) is recomputed after the assertion occurs here. This basically throws all determinism in tests out of whack - I wonder if this'll end up leading to spurious failures over time.

Copy link
Member

Choose a reason for hiding this comment

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

@VasylMarchuk probably okay to merge this for now, could you please add a comment to all of these saying // Investigate why clickable() doesn't call settled() or something like that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes sounds like a good idea, I also found having to add explicit settled calls weird and suspicious.

It feels like some async action is happening, which isn't getting included in the ember run-loop, and isn't tracked automatically by test waiters, then it registers a promise or a Ember.run.later, and that promise is awaited on by the extra settled call. Maybe it's the new RequestManager misbehaving, maybe it's our own async logic somewhere.

Also in one of my previous projects, we had to manually call waitForPromise from @ember/test-waiters to get our tests to wait for our custom async logic, like this:

Знімок екрана 2023-11-16 о 14 14 58

https://github.com/emberjs/ember-test-waiters/blob/master/addon/addon/wait-for-promise.ts

Copy link
Collaborator Author

@VasylMarchuk VasylMarchuk Nov 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

But also, on the other hand, what's strange is that we have 277 tests, but only 8 tests were failing and I needed to add only 9 extra await settled() calls in 7 files...

Will definitely add a comment and maybe worth an extra investigation in a separate PR


assert.strictEqual(blocksPage.editableBlocks.length, 2, 'expected 2 editable blocks to be present');

await blocksPage.insertBlockMarkers[0].click();
Expand Down
2 changes: 2 additions & 0 deletions tests/acceptance/concepts-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import { setupApplicationTest } from 'ember-qunit';
import { setupMirage } from 'ember-cli-mirage/test-support';
import { setupWindowMock } from 'ember-window-mock/test-support';
import { signInAsStaff } from 'codecrafters-frontend/tests/support/authentication-helpers';
import { setupAnimationTest } from 'ember-animated/test-support';

function createConcepts(server) {
createConceptFromFixture(server, tcpOverview);
Expand All @@ -19,6 +20,7 @@ function createConcepts(server) {

module('Acceptance | concepts-test', function (hooks) {
setupApplicationTest(hooks);
setupAnimationTest(hooks);
setupMirage(hooks);
setupWindowMock(hooks);

Expand Down
5 changes: 5 additions & 0 deletions tests/acceptance/course-admin/activate-tester-version-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { module, test } from 'qunit';
import { setupApplicationTest } from 'ember-qunit';
import { setupMirage } from 'ember-cli-mirage/test-support';
import { settled } from '@ember/test-helpers';
import { signInAsStaff } from 'codecrafters-frontend/tests/support/authentication-helpers';
import testerVersionsPage from 'codecrafters-frontend/tests/pages/course-admin/tester-versions-page';
import testScenario from 'codecrafters-frontend/mirage/scenarios/test';
Expand Down Expand Up @@ -45,9 +46,13 @@ module('Acceptance | course-admin | activate-tester-version', function (hooks) {
assert.ok(testerVersionsPage.testerVersionListItem[0].activateButton.isPresent);

await testerVersionsPage.testerVersionListItem[0].activateButton.click();
await settled();

assert.notOk(testerVersionsPage.testerVersionListItem[0].activateButton.isPresent);

await testerVersionsPage.testerVersionListItem[1].activateButton.click();
await settled();

assert.ok(testerVersionsPage.testerVersionListItem[0].activateButton.isPresent);
assert.notOk(testerVersionsPage.testerVersionListItem[1].activateButton.isPresent);

Expand Down
3 changes: 3 additions & 0 deletions tests/acceptance/course-admin/apply-update-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { module, test } from 'qunit';
import { setupApplicationTest } from 'ember-qunit';
import { setupMirage } from 'ember-cli-mirage/test-support';
import { settled } from '@ember/test-helpers';
import { signInAsStaff } from 'codecrafters-frontend/tests/support/authentication-helpers';
import updatesPage from 'codecrafters-frontend/tests/pages/course-admin/updates-page';
import updatePage from 'codecrafters-frontend/tests/pages/course-admin/update-page';
Expand Down Expand Up @@ -34,6 +35,8 @@ module('Acceptance | course-admin | apply-update', function (hooks) {

assert.ok(updatePage.applyUpdateButton.isPresent);
await updatePage.applyUpdateButton.click();
await settled();

assert.notOk(updatePage.applyUpdateButton.isPresent);
});

Expand Down
4 changes: 3 additions & 1 deletion tests/acceptance/course-admin/view-tester-versions-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import percySnapshot from '@percy/ember';
import testerVersionsPage from 'codecrafters-frontend/tests/pages/course-admin/tester-versions-page';
import testScenario from 'codecrafters-frontend/mirage/scenarios/test';
import { currentURL } from '@ember/test-helpers';
import { currentURL, settled } from '@ember/test-helpers';
import { module, test } from 'qunit';
import { setupApplicationTest } from 'ember-qunit';
import { setupMirage } from 'ember-cli-mirage/test-support';
Expand Down Expand Up @@ -81,6 +81,8 @@ module('Acceptance | course-admin | view-tester-versions', function (hooks) {
});

await testerVersionsPage.clickOnSyncWithGitHubButton();
await settled();

assert.strictEqual(testerVersionsPage.testerVersionListItem.length, 2, 'should have 2 tester versions');
});

Expand Down
3 changes: 3 additions & 0 deletions tests/acceptance/course-admin/view-update-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { module, test } from 'qunit';
import { setupApplicationTest } from 'ember-qunit';
import { setupMirage } from 'ember-cli-mirage/test-support';
import { settled } from '@ember/test-helpers';
import { signInAsStaff } from 'codecrafters-frontend/tests/support/authentication-helpers';
import updatesPage from 'codecrafters-frontend/tests/pages/course-admin/updates-page';
import updatePage from 'codecrafters-frontend/tests/pages/course-admin/update-page';
Expand Down Expand Up @@ -78,6 +79,8 @@ module('Acceptance | course-admin | view-update', function (hooks) {
update.update('definitionFileContentsDiff', '+ updated diff');

await updatePage.clickOnSyncWithGitHubButton();
await settled();

assert.ok(updatePage.fileContentsDiff.text.includes('+ updated diff'), 'diff should be updated after syncing with github');
});

Expand Down
4 changes: 3 additions & 1 deletion tests/acceptance/course-admin/view-updates-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import percySnapshot from '@percy/ember';
import testScenario from 'codecrafters-frontend/mirage/scenarios/test';
import updatesPage from 'codecrafters-frontend/tests/pages/course-admin/updates-page';
import { currentURL } from '@ember/test-helpers';
import { currentURL, settled } from '@ember/test-helpers';
import { module, test } from 'qunit';
import { setupApplicationTest } from 'ember-qunit';
import { setupMirage } from 'ember-cli-mirage/test-support';
Expand Down Expand Up @@ -107,6 +107,8 @@ module('Acceptance | course-admin | view-updates', function (hooks) {
});

await updatesPage.clickOnSyncWithGitHubButton();
await settled();

assert.strictEqual(updatesPage.updateListItems.length, 2, 'should have 2 updates');
});

Expand Down
14 changes: 13 additions & 1 deletion tests/acceptance/course-page/publish-to-github-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import catalogPage from 'codecrafters-frontend/tests/pages/catalog-page';
import testScenario from 'codecrafters-frontend/mirage/scenarios/test';
import window from 'ember-window-mock';
import { module, test } from 'qunit';
import { setupAnimationTest } from 'ember-animated/test-support';
import { setupAnimationTest, animationsSettled } from 'ember-animated/test-support';
import { setupApplicationTest } from 'ember-qunit';
import { setupMirage } from 'ember-cli-mirage/test-support';
import { setupWindowMock } from 'ember-window-mock/test-support';
Expand Down Expand Up @@ -61,8 +61,13 @@ module('Acceptance | course-page | publish-to-github-test', function (hooks) {
await catalogPage.visit();
await catalogPage.clickOnCourse('Build your own Redis');
await coursePage.repositoryDropdown.click();

await animationsSettled();

await coursePage.repositoryDropdown.clickOnAction('Publish to GitHub');

await animationsSettled();

assert.ok(coursePage.configureGithubIntegrationModal.isOpen, 'configure github modal is open');
assert.ok(coursePage.configureGithubIntegrationModal.fixGitHubAppInstallationPrompt.isVisible, 'fix github app installation prompt is visible');

Expand All @@ -75,6 +80,8 @@ module('Acceptance | course-page | publish-to-github-test', function (hooks) {

await coursePage.configureGithubIntegrationModal.fixGitHubAppInstallationPrompt.refreshStatusButton.click();

await animationsSettled();

assert.notOk(
coursePage.configureGithubIntegrationModal.fixGitHubAppInstallationPrompt.isVisible,
'fix github app installation prompt is not visible',
Expand Down Expand Up @@ -102,8 +109,13 @@ module('Acceptance | course-page | publish-to-github-test', function (hooks) {

window.confirm = () => true;

await animationsSettled();

await coursePage.configureGithubIntegrationModal.clickOnPublishButton();
await coursePage.configureGithubIntegrationModal.clickOnDisconnectRepositoryButton();

await animationsSettled();

await coursePage.configureGithubIntegrationModal.clickOnPublishButton();
});
});
2 changes: 2 additions & 0 deletions tests/acceptance/course-page/start-course-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,8 @@ module('Acceptance | course-page | start-course', function (hooks) {
await animationsSettled();

baseRequestsCount += 2; // For some reason, we're rendering the "Request Other" button again when a language is chosen.
baseRequestsCount += 1; // An extra request for leaderboard-entries started happening after ember-data upgrade

assert.strictEqual(apiRequestsCount(this.server), baseRequestsCount + 1, 'create repository request was executed');

assert.strictEqual(coursePage.createRepositoryCard.expandedSectionTitle, 'Language Proficiency', 'current section title is language proficiency');
Expand Down
15 changes: 12 additions & 3 deletions tests/acceptance/course-page/try-other-language-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ module('Acceptance | course-page | try-other-language', function (hooks) {
'fetch repositories (course page)',
'fetch leaderboard entries (course page)',
'fetch languages (course page)',
'fetch leaderboard entries (after ember-data upgrade)',
].length;

assert.strictEqual(apiRequestsCount(this.server), expectedRequestsCount, `expected ${expectedRequestsCount} requests`);
Expand All @@ -69,7 +70,15 @@ module('Acceptance | course-page | try-other-language', function (hooks) {
await coursePage.createRepositoryCard.clickOnLanguageButton('Go');
await animationsSettled();

assert.strictEqual(apiRequestsCount(this.server), expectedRequestsCount + 3, `expected ${expectedRequestsCount + 3} requests`); // fetch languages, requests + Create repository request
//prettier-ignore
expectedRequestsCount += [
'fetch languages',
'fetch requests',
'create repository',
'fetch leaderboard entries',
].length;

assert.strictEqual(apiRequestsCount(this.server), expectedRequestsCount, `expected ${expectedRequestsCount} requests`);
assert.strictEqual(coursePage.repositoryDropdown.activeRepositoryName, 'Go', 'Repository name should change');
assert.strictEqual(currentURL(), '/courses/redis/introduction?repo=2', 'current URL is course page URL with repo query param');

Expand All @@ -89,12 +98,12 @@ module('Acceptance | course-page | try-other-language', function (hooks) {
repository.update({ lastSubmission: this.server.create('submission', { repository }) });

await Promise.all(window.pollerInstances.map((poller) => poller.forcePoll()));
assert.strictEqual(apiRequestsCount(this.server), expectedRequestsCount + 8, 'polling should have run');
assert.strictEqual(apiRequestsCount(this.server), expectedRequestsCount + 5, 'polling should have run');

assert.ok(coursePage.repositorySetupCard.statusIsComplete, 'current status is complete');

await Promise.all(window.pollerInstances.map((poller) => poller.forcePoll()));
assert.strictEqual(apiRequestsCount(this.server), expectedRequestsCount + 10, 'polling should have run again');
assert.strictEqual(apiRequestsCount(this.server), expectedRequestsCount + 7, 'polling should have run again');
});

test('can try other language from repository setup page (regression)', async function (assert) {
Expand Down
6 changes: 5 additions & 1 deletion tests/acceptance/manage-membership-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import catalogPage from 'codecrafters-frontend/tests/pages/catalog-page';
import { formatWithOptions } from 'date-fns/fp';
import membershipPage from 'codecrafters-frontend/tests/pages/membership-page';
import testScenario from 'codecrafters-frontend/mirage/scenarios/test';
import { currentURL } from '@ember/test-helpers';
import { currentURL, settled } from '@ember/test-helpers';

module('Acceptance | manage-membership-test', function (hooks) {
setupApplicationTest(hooks);
Expand Down Expand Up @@ -82,6 +82,8 @@ module('Acceptance | manage-membership-test', function (hooks) {
assert.strictEqual(membershipPage.cancelSubscriptionModal.cancelButtonText, 'Cancel Trial');

await membershipPage.cancelSubscriptionModal.clickOnCancelSubscriptionButton();
await settled();

assert.notOk(membershipPage.cancelSubscriptionModal.isVisible);

assert.strictEqual(membershipPage.membershipPlanSection.descriptionText, 'Your CodeCrafters membership is currently inactive.');
Expand Down Expand Up @@ -112,6 +114,8 @@ module('Acceptance | manage-membership-test', function (hooks) {
assert.strictEqual(membershipPage.cancelSubscriptionModal.cancelButtonText, 'Cancel Subscription');

await membershipPage.cancelSubscriptionModal.clickOnCancelSubscriptionButton();
await settled();

assert.notOk(membershipPage.cancelSubscriptionModal.isVisible);

assert.strictEqual(
Expand Down
Loading