emberjs / ember.js

Ember.js - A JavaScript framework for creating ambitious web applications
https://emberjs.com
MIT License
22.46k stars 4.21k forks source link

5.10: test waiters broken? #20724

Closed Turbo87 closed 1 month ago

Turbo87 commented 1 month ago

🐞 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 https://github.com/rust-lang/crates.io/pull/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

➕ Additional Context

NullVoxPopuli commented 1 month ago

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

jelhan commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

@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 commented 1 month ago

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, c7508cc64fdd416962aa47cb963ed36d033eb8cf 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: ea6144251b919db1af5e8c96e2a6de77026e1ac2

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 ea6144251b919db1af5e8c96e2a6de77026e1ac2 is what broke waiters -- so now we can start to poke around that set of changes

ef4 commented 1 month ago

20726 should be a complete fix for this.

kategengler commented 1 month ago

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

jelhan commented 1 month ago

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.