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

Assert: Improve detection of bad calls to assert.async() callbacks #1642

Merged
merged 10 commits into from
Aug 2, 2021

Conversation

Krinkle
Copy link
Member

@Krinkle Krinkle commented Jul 29, 2021

Background

When creating two async pauses in a test, it was possible for a test to pass by invoking one of them twice, and the other not at all.

Easy scenario (though perhaps not realistic):

Use assert.async() twice, assigned as done1 and done2 in the same QUnit.test() case, and then simulate the failure scenario such that you wrongly call done1 two times, and forget to call done2.

Complex scenario across QUnit.test() and "afterEach" hooks, since these previously shared a single semaphore:

Use assert.async() once in a simple test, and schedule the resume call in the future, but then fail with an uncaught error. The uncaught error is found and Test.run() would internally kill the pause by resetting the semaphore to zero (this make sense since we shouldn't wait for the release once the test is known to have failed).
After this reset, we proceed to the "afterEach" hook. Suppose this hook is also async, and during its execution, the originally scheduled resume call happens. This would effectively end up releasing the afterEach's async pause despite not being finished yet, and then we proceed to the next test. That test would then fail when the afterEach's own release call happens, failing as "release during a different test".

This is the scenario of #1432.

Fix this and numerous other edge cases by making the returned callbacks from assert.async() strict about which locks they release.

Each lock now adds a unique token to a map, and invoking the release function decrements/removes this token from the map.

Notes

  • es6-map.js assigns the fallback in all browsers.
    This is a bug, to be fixed later.

  • The isNaN(semaphore) logic was originally added in 2015 by ea3e350.
    At the time, the internal resume function was public, and NaN could emerge through QUnit.start("bla") as result of semaphore += "bla". This has not been possible for a while. During PR Test: increase code coverage #1590, I did not trace the origin of this code, and thus did not realize that it was already obsolete (the semaphore itself is not publicly supported).

  • The "during different test" error is now almost impossible to trigger since we now kill pending locks during test failures and
    tolerate all late calls equally. This meant the drooling-done.js test case now fails in a more limited way.

    I added a new test case for coverage, that reproduces it still, but it's a lot more obscure – it requires the original test to pass and then also have an unexpected call during a different test.

  • I considered using the phrase "async lock" in the public-facing error messages, but found this perhaps too internal/technical when coming from the perspective of var done = assert.async();.

    In order to keep the code shared between handling of async-await, Promise, and assert.async, but remain friendly and understandable, I went for the phrase "async pause".

Fixes #1432.

@Krinkle Krinkle requested a review from smcclure15 July 29, 2021 01:26
== Background ==

When creating two async pauses in a test, it was possible for a test
to pass by invoking one of them twice, and the other not at all.

Easy scenario (though perhaps not realistic):

> Use `assert.async()` twice, assigned as done1 and done2 in the same
> `QUnit.test()` case, and then simulate the failure scenario such that
> you wrongly call done1 two times, and forget to call done2.

Complex scenario across `QUnit.test()` and "afterEach" hooks, since
these previously shared a single semaphore:

> Use `assert.async()` once in a simple test, and schedule the resume
> call in the future, but then fail with an uncaught error. The uncaught
> error is found and `Test.run()` would internally kill the pause by
> resetting the semaphore to zero (this make sense since we shouldn't
> wait for the release once the test is known to have failed).
> After this reset, we proceed to the "afterEach" hook. Suppose this
> hook is also async, and during its execution, the originally scheduled
> resume call happens. This would effectively end up releasing the
> afterEach's async pause despite not being finished yet, and then we
> proceed to the next test. That test would then fail when the afterEach's
> own release call happens, failing as "release during a different test".

This is the scenario of #1432.

Fix this and numerous other edge cases by making the returned
callbacks from `assert.async()` strict about which locks they release.

Each lock now adds a unique token to a map, and invoking the release
function decrements/removes this token from the map.

== Notes ==

* es6-map.js assigns the fallback in all browsers.
  This is a bug, to be fixed later.

* The `isNaN(semaphore)` logic was originally added in 2015 by ea3e350.
  At the time, the internal resume function was public, and NaN could
  emerge through `QUnit.start("bla")` as result of `semaphore += "bla"`.
  This has not been possible for a while. During PR #1590, I did not
  trace the origin of this code, and thus did not realize that it was
  already obsolete (the semaphore itself is not publicly supported).

* The "during different test" error is now almost impossible to
  trigger since we now kill pending locks during test failures and
  tolerate all late calls equally. This meant the `drooling-done.js`
  test case now fails in a more limited way.

  I added a new test case for coverage, that reproduces it still, but
  it's a lot more obscure – it requires the original test to pass and
  then also have an unexpected call during a different test.

* I considered using the phrase "async lock" in the public-facing
  error messages, but found this perhaps too internal/technical
  when coming from the perspective of `var done = assert.async();`.

  In order to keep the code shared between handling of async-await,
  Promise, and assert.async, but remain friendly and understandable,
  I went for the phrase "async pause".

Fixes #1432.
Copy link
Member

@smcclure15 smcclure15 left a comment

Choose a reason for hiding this comment

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

I know my domain has some test utilities that sniff at that semaphore bit to get really picky about choosing to throw or continue in different scenarios. Some areas use QUnit.log and upon a failure, if they are in a pause situation, they want to jump to the next test so it's not left in a bad state. All of which I anticipate this handles much more gracefully after all.

That semaphore logic is completely implementation detail so that's not fair to hold up this PR, nor trigger a major version, just noting that dependency for us and others that may have found their way to relying on that.

src/test.js Outdated Show resolved Hide resolved
src/test.js Outdated Show resolved Hide resolved
src/test.js Outdated Show resolved Hide resolved
src/cli/run.js Outdated Show resolved Hide resolved
Krinkle and others added 4 commits July 29, 2021 17:09
Co-authored-by: Steve McClure <smcclure15@gmail.com>
Co-authored-by: Steve McClure <smcclure15@gmail.com>
Copy link
Member

@gibson042 gibson042 left a comment

Choose a reason for hiding this comment

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

Nice!

Comment on lines +4 to 6
// FIXME: This check is broken. This file is embedded in the qunit.js closure,
// thus the Map var is hoisted in that scope, and starts undefined (not a function).
var Map = typeof Map === "function" ? Map : function StringMap() {
Copy link
Member

Choose a reason for hiding this comment

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

Should we just fix it now?

Suggested change
// FIXME: This check is broken. This file is embedded in the qunit.js closure,
// thus the Map var is hoisted in that scope, and starts undefined (not a function).
var Map = typeof Map === "function" ? Map : function StringMap() {
if ( typeof Map !== "function" ) {
var Map = function StringMap() {

Copy link
Member Author

Choose a reason for hiding this comment

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

That won't work, it'll still be hosted just the same, right? It's a pretty tough situation. I set it up such that it is included by rollup as intro (within the file closure, so as to not leak and be seen by end-user source code), and thus the variable will be seen by fuzzysort.js and other code we use naturally as if it was a global.

Without pulling in globalThis into here, I'm not sure how to do this because as soon as you declare anything as Map you inherently also deprive your ability to see the outer scope's value for that same reference.

Copy link
Member

@gibson042 gibson042 Jul 29, 2021

Choose a reason for hiding this comment

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

Oh, right. What's needed is something like the following, which can't be fixed at this level (and is out of scope for this PR).

(function(, Map, ) {
	if ( typeof Map !== "function" ) {
		Map = function StringMap() {};
	}
	
})(, typeof Map === "function" && Map, )

src/test.js Outdated Show resolved Hide resolved
src/test.js Outdated Show resolved Hide resolved
src/test.js Outdated Show resolved Hide resolved
No need to rely on global state for this. This also makes sure it
is always reported under the correct test. Either way, we still get
a last-minute check in pushFailure->pushResult that it is the
current test.
@Krinkle Krinkle merged commit 9444070 into main Aug 2, 2021
@Krinkle Krinkle deleted the semaphore-map branch August 2, 2021 22:18
Krinkle added a commit that referenced this pull request Aug 2, 2021
Krinkle added a commit to Krinkle/qunit that referenced this pull request Feb 8, 2024
Follows-up 163c9bc (qunitjs#1642),
which changed an internalRecover() to internalStart(), whereas
internalStart will (correctly) not resume if there are other pauses
still remaining.

Change this back to internalRecover().

Fixes qunitjs#1705.
Krinkle added a commit that referenced this pull request Feb 9, 2024
Follows-up 163c9bc (#1642),
which changed an internalRecover() to internalStart(), whereas
internalStart will (correctly) not resume if there are other pauses
still remaining.

Change this back to internalRecover().

Fixes #1705.
Closes #1739.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Exception in one test can cause trailing failures
3 participants