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

5.10: test waiters broken? #20724

Closed
Turbo87 opened this issue Jul 23, 2024 · 8 comments
Closed

5.10: test waiters broken? #20724

Turbo87 opened this issue Jul 23, 2024 · 8 comments

Comments

@Turbo87
Copy link
Member

Turbo87 commented Jul 23, 2024

🐞 Describe the Bug

After upgrading the crates.io codebase from 5.9 to 5.10 several tests started failing for no apparent reason. It looks like the test waiters are not working as intended and are causing the tests to be flaky. When I run the test suite locally the subset of failing tests is different from CI, which indicates that race conditions might be responsible.

🔬 Minimal Reproduction

see rust-lang/crates.io#9035

😕 Actual Behavior

Previously succeeding tests are failing.

🤔 Expected Behavior

Previously succeeding tests should keep passing, if the test waiter system is used correctly.

🌍 Environment

  • Ember: 5.10.1
  • Ember-CLI: 5.10.0
  • Node.js/npm: 20.15.1
  • OS: Linux & MacOS
  • Browser: Chrome & Firefox

➕ Additional Context

@NullVoxPopuli
Copy link
Contributor

NullVoxPopuli commented Jul 23, 2024

This matches what i've been seeing in other open source repos, too

@jelhan
Copy link
Contributor

jelhan commented Jul 29, 2024

I noticed the same in one app. The click test helper resolves before a network request triggered by that click has settled. The network request is done with fetch. The project uses ember-fetch@8.1.2. fetch is imported from fetch package in the file doing the network request.

The tests are working as expected with ember-source@5.9.0. They are starting to fail if upgrading ember-source to 5.10.1. I don't see any entry in the changelog for 5.10.0 and 5.10.1, which may be related.

@NullVoxPopuli
Copy link
Contributor

Looks like it may have been the legacy waiters that broke:
image
(this is from the @ember/test-helpers test suite)

@jelhan do you have a minimal repro of the click resolving too early?

@jelhan
Copy link
Contributor

jelhan commented Jul 29, 2024

@jelhan do you have a minimal repro of the click resolving too early?

Sadly not. I expect reproducing is not that easy. Only 2 out of many tests in the app are affected.

Aren't the failing tests in @ember/ember-test-helpers enough to reproduce and debug?

@NullVoxPopuli
Copy link
Contributor

NullVoxPopuli commented Jul 29, 2024

Aren't the failing tests in @ember/ember-test-helpers enough to reproduce and debug?

they are, it's just test-helpers don't exactly provide minimal reproductions -- there is a lot of its own code to wade through as we look at the underling low level waiter infra.


Been bisecting ember-source, but I fell in to a local minima,
c7508cc is what bisect told me is the problem:

TypeError: Cannot set property default of #<Object> which has only a getter
...
Error: Could not find module `ember-cli-test-loader/test-support/index` imported from `dummy/tests/test-helper`

which seems like it was fixed later.
So, doing bisect again, but treating c75 as a "good" commit, maybe the real culprit can be found:

# on main
git bisect start
git bisect bad
git bisect good c7508cc64fdd416962aa47cb963ed36d033eb8cf

Looks like we've arrived at: ea61442

Bisecting: 0 revisions left to test after this (roughly 0 steps)
[ea6144251b919db1af5e8c96e2a6de77026e1ac2] This makes Ember's test suite build with Vite, with no AMD loader present. Which ensures that Ember is strictly following the ES module spec.

Gonna run the bisect again, to make extra sure, but this time with narrower range

# on main 
git bisect start
git bisect bad ea6144251b919db1af5e8c96e2a6de77026e1ac2
git bisect good c7508cc64fdd416962aa47cb963ed36d033eb8cf

which re-confirms that ea61442 is what broke waiters -- so now we can start to poke around that set of changes

@ef4
Copy link
Contributor

ef4 commented Aug 5, 2024

#20726 should be a complete fix for this.

@kategengler
Copy link
Member

This should be fixed in v5.10.2 and in v5.11.0-beta.2

@jelhan
Copy link
Contributor

jelhan commented Aug 6, 2024

This should be fixed in v5.10.2 and in v5.11.0-beta.2

I can confirm that v5.10.2 is working as expected in the app which was affected by the bug before.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants