Closed Bug 1672237 Opened 3 years ago Closed 3 years ago

Do a better job of catching exceptions during tests

Categories

(Core :: Panning and Zooming, task)

Product:
Core
Component:
Panning and Zooming
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
84 Branch
Tracking Flags:
Tracking Status
firefox84 --- fixed

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(9 files)

Bug 1672237 - Introduce a SimpleTest.finishWithFailure wrapper. r?botond
47 bytes, text/x-phabricator-request
Details | Review
Bug 1672237 - Use SimpleTest.finishWithFailure in various APZ mochitests. r?botond
47 bytes, text/x-phabricator-request
Details | Review
Bug 1672237 - Update most remaining APZ mochitests to use SimpleTest.finishWithFailure. r?botond
47 bytes, text/x-phabricator-request
Details | Review
Bug 1672237 - Add a subtestFailed function, similar to SimpleTest.finishWithFailure. r?botond
47 bytes, text/x-phabricator-request
Details | Review
Bug 1672237 - Replace finally(subtestDone) with then(subtestDone, subtestFailed). r?botond
47 bytes, text/x-phabricator-request
Details | Review
Bug 1672237 - Replace then(subtestDone) with then(subtestDone, subtestFailed). r?botond
47 bytes, text/x-phabricator-request
Details | Review
Bug 1672237 - Ensure cleanup is done regardless of success or failure. r?botond
47 bytes, text/x-phabricator-request
Details | Review
Bug 1672237 - Add a subtestDone to the fission helper. r?botond
47 bytes, text/x-phabricator-request
Details | Review
Bug 1672237 - Update fission helpers to use subtestFailed. r?botond
47 bytes, text/x-phabricator-request
Details | Review

A lot of our tests end with things like .then(test).finally(SimpleTest.finish). If an exception happens during test, then this will discard the exception and finish the test anyway. While great at preventing harness hangs/timeouts, this is not so great if we actually want to trigger failures on exceptions. It would be better to have an explicit failure in these cases.

I ran into a similar-ish problem in 1528937 comment 36 which inspired to me look into this more, although what I have in mind here probably won't directly address that problem.

Note also that many of our tests use a continuation-style .then(runContinuation(test)).finally(SimpleTest.finish) where the runContinuation wrapper will catch exceptions from the test function. However as we move away from that continuation style more of our tests will be subject to this "exceptions get dropped" problem.

This updates APZ mochitests that are using .then(SimpleTest.finish) or
.finally(SimpleTest.finish) to instead pass the rejection/exception case
to SimpleTest.finishWithFailure.

Depends on D94161

A few tests do the SimpleTest.waitForExplicitFinish/SimpleTest.finish dance
without a promise/async chain. This updates those tests to follow a more
async style and to send failures through SimpleTest.finishWithFailure.

Note that the conversion to async is done "locally" by wrapping callback-taking
helper function calls in promises. In some cases it probably makes sense to
modify the helper function itself to return a promise, but that's out of scope
for this bug, so I'm punting those kinds of changes to a follow-up bug.

Depends on D94162

Unrelated robustification that I saw while writing the previous patch.

Depends on D94166

Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2a3da8e5d6a9
Introduce a SimpleTest.finishWithFailure wrapper. r=botond
https://hg.mozilla.org/integration/autoland/rev/e2a78f81a8bd
Use SimpleTest.finishWithFailure in various APZ mochitests. r=botond
https://hg.mozilla.org/integration/autoland/rev/5b41917730e2
Update most remaining APZ mochitests to use SimpleTest.finishWithFailure. r=botond
https://hg.mozilla.org/integration/autoland/rev/85b2a686add3
Add a subtestFailed function, similar to SimpleTest.finishWithFailure. r=botond
https://hg.mozilla.org/integration/autoland/rev/dc1d17e222a7
Replace finally(subtestDone) with then(subtestDone, subtestFailed). r=botond
https://hg.mozilla.org/integration/autoland/rev/95429d127456
Replace then(subtestDone) with then(subtestDone, subtestFailed). r=botond
https://hg.mozilla.org/integration/autoland/rev/8d711bceb742
Ensure cleanup is done regardless of success or failure. r=botond
https://hg.mozilla.org/integration/autoland/rev/1633d4ce1773
Add a subtestDone to the fission helper. r=botond
https://hg.mozilla.org/integration/autoland/rev/718f9800877a
Update fission helpers to use subtestFailed. r=botond
You need to log in before you can comment on or make changes to this bug.