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.

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

Upgrade Ember Data to version 4.12.4 #974

Merged
merged 8 commits into from
Nov 16, 2023
Merged

Conversation

VasylMarchuk
Copy link
Collaborator

@VasylMarchuk VasylMarchuk commented Nov 9, 2023

Brief

This is a pull request to upgrade ember-data from 4.11.3 to 4.12.4

Details

After upgrading to latest ember-data@4.12.4, AbortController was missing during FastBoot-enabled builds and causing a build error. It had to be added to FastBoot's "sandbox globals". In addition, some of the tests started failing and had to be adjusted or fixed:

  • Added AbortController to FastBoot's buildSandboxGlobals
  • Sped up concepts tests by adding setupAnimationTest call
  • Fixed some failing tests by adding an extra await settled() call
  • Fixed some failing tests by adding an extra await animationsSettled() call
  • Fixed some failing tests that were counting API requests incorrectly

Checklist

  • I've thoroughly self-reviewed my changes
  • I've added tests for my changes, unless they affect admin-only areas (or are otherwise not worth testing)
  • I've verified any visual changes using Percy (add a commit with [percy] in the message to trigger)

@VasylMarchuk VasylMarchuk added the dependencies Pull requests that update a dependency file label Nov 9, 2023
@VasylMarchuk VasylMarchuk self-assigned this Nov 9, 2023
@VasylMarchuk VasylMarchuk changed the title Upgrade Ember Data to version 4.12.3 Upgrade Ember Data to version 4.12.4 Nov 9, 2023
Copy link

github-actions bot commented Nov 9, 2023

Test Results

    1 files  ±0      1 suites  ±0   3m 8s ⏱️ -9s
272 tests ±0  271 ✔️ ±0  1 💤 ±0  0 ±0 
277 runs  ±0  276 ✔️ ±0  1 💤 ±0  0 ±0 

Results for commit a0858f5. ± Comparison against base commit 08807a8.

♻️ This comment has been updated with latest results.

Copy link

codecov bot commented Nov 9, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (08807a8) 75.34% compared to head (a0858f5) 75.09%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #974      +/-   ##
==========================================
- Coverage   75.34%   75.09%   -0.25%     
==========================================
  Files         351      351              
  Lines        3265     3265              
  Branches      274      274              
==========================================
- Hits         2460     2452       -8     
- Misses        713      718       +5     
- Partials       92       95       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@VasylMarchuk VasylMarchuk marked this pull request as ready for review November 16, 2023 10:05
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

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

@@ -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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants