Some mochitest flavors can fail silently due a second less-useful unload-event-handler invocation, if the test navigates
Categories
(Testing :: Mochitest, defect, P1)
Tracking
(firefox118 fixed)
Tracking | Status | |
---|---|---|
firefox118 | --- | fixed |
People
(Reporter: dholbert, Assigned: dholbert)
References
Details
Attachments
(3 files)
I've been poking at bug 1845221 to see what was going on and causing silent failure-count-increments-without-any-logging there, and I think I've got to essentially the root of the issue.
Here's what's going on here:
(1) If a test reloads (or navigates to its own URL) -- as test_link.html
from bug 1845221 did, by clicking a link which navigated to the same document -- then we end up firing any registered unload handlers twice. I confirmed this in a trivial a11y mochitest that I wrote which registers its own unload handler.
(2) When this happens: on the second unload handler invocation, **a bunch of data is missing or cleaned up on the window
object. In particular: testwin.SimpleTest._tests.length
is 0
and testwin.SimpleTest.testsLength
is undefined
.
(3) This causes our logic for detecting test-results-logged-after-finish to be confused, since it compares and subtracts those^ two values. The subtraction produces a wrongtestlength
of NaN, and so we end up skipping the for (var i = 0; i < wrongtestlength; i++) {
loop, and so we skip the logging, but we still increment the failure-count via TestRunner.updateUI([{ result: false }]);
This means we end up with a failure-count of 1 but with no logging to help out with diagnosing the failure.
I just ran across a patch that landed yesterday to address this in Bug 1844310, which added a check for this and some logging -- but I think we can make the check and the logging a little more targeted and actionable. (In particular, we can check testsLength
directly; and we should log about the unload handler being invoked redundantly possibly due to navigation during the test, rather than logging after finish. Since, as described above, the problem is not actually that we're logging after finish. It just happens to be that we're hitting an error scenario during the second time we go through the code that checks for logging after finish.)
So: per bug 1844310 comment 9, I think we should probably back out the patch there and land a patch that I'll be posting here shortly.
Assignee | ||
Updated•2 years ago
|
Updated•2 years ago
|
Assignee | ||
Comment 1•2 years ago
|
||
This patch doesn't affect behavior; it's just splitting one compound check into
two nested ones, and adjusting the indentation.
I'm doing this because the next patch will adjust the inner logic, and
splitting it up this way lets me do that in a more targeted way.
Assignee | ||
Comment 2•2 years ago
|
||
As described in the bug, this can sometimes happen when mochitests navigate,
e.g. due to synthesizing a click on a link to the same document.
Assignee | ||
Comment 3•2 years ago
|
||
Assignee | ||
Comment 4•2 years ago
|
||
part 3 (just added) is just to be sure we cover all our bases, in case there's another independent way of triggering this same silent failure.
Assignee | ||
Comment 5•2 years ago
|
||
I'll do some try runs with these patches and with test_link.html
added back (as a canary that's expected to trip over the new check from part 2). Assuming everything looks good, we can request for bug 1844310 to be backed out and then land the stack here.
Assignee | ||
Comment 6•2 years ago
|
||
Try runs look good.
Here are several try runs, layered on top of central with bug 1844310 backed out, and then with patches as-described below:
(1) With this patch stack (green, modulo known/unrelated intermittents):
https://treeherder.mozilla.org/jobs?repo=try&revision=086ba65df462e88131020cf72ea775d99de20acb
(2) With this patch stack, plus the known-problematic test_link.html
a11y mochitest added back -- good news, it does successfully trigger the new logging:
https://treeherder.mozilla.org/jobs?repo=try&revision=aaac90ed9f820f8c97c548d26ba2a655a6add607
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/actions/test_link.html fired an unload callback with missing test data, possibly due to the test navigating or reloading
(3) ...and now with the undefined
-handling case disabled, to see if the "cover all our bases" part-3 fallback-handling kicks in and works properly (good news, it does):
https://treeherder.mozilla.org/jobs?repo=try&revision=20fd25285280419f69be66be8a117b613ba4899a
Comment 9•2 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/df7c67c3519e
https://hg.mozilla.org/mozilla-central/rev/b78a2a91f0d4
https://hg.mozilla.org/mozilla-central/rev/5f201489618f
Description
•