Closed
Bug 1266824
Opened 8 years ago
Closed 8 years ago
Intermittent test_ext_alarms.html | alarm fired within expected time
Categories
(WebExtensions :: Untriaged, defect, P2)
WebExtensions
Untriaged
Tracking
(firefox48 fixed, firefox49 fixed)
People
(Reporter: KWierso, Assigned: bsilverberg)
Details
(Keywords: intermittent-failure, Whiteboard: triaged)
Attachments
(1 file)
Comment hidden (Intermittent Failures Robot) |
Comment 2•8 years ago
|
||
started after we turned on tests on android...potentially race. want to run on try as hard to run on android (emulator multi-step complexity)...talk to Matt possibly about android issues.
Assignee: nobody → bob.silverberg
Priority: -- → P2
Whiteboard: triaged
Assignee | ||
Comment 3•8 years ago
|
||
test_alarm_fires_with_when is failing intermittently and it looks like the function inside the setTimeout (which is designed to detect that the alarm did not fire as expected) is firing, even though the test is passing and the alarm did fire. I added a condition around it so it will only mark the test as failed if the alarm did not in fact fire. I also did some clean up of the existing tests, including adding this type of logic to all tests that expect an alarm or alarms to have fired. Review commit: https://reviewboard.mozilla.org/r/48827/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/48827/
Attachment #8745089 -
Flags: review?(aswan)
Assignee | ||
Updated•8 years ago
|
Status: NEW → ASSIGNED
Iteration: --- → 49.1 - May 9
Assignee | ||
Comment 4•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=64a86a2a4fc3
Comment 5•8 years ago
|
||
Comment on attachment 8745089 [details] MozReview Request: Bug 1266824 - Intermittent test_ext_alarms.html | alarm fired within expected time, r?aswan https://reviewboard.mozilla.org/r/48827/#review45833 ::: toolkit/components/extensions/test/mochitest/test_ext_alarms.html:43 (Diff revision 1) > let ALARM_NAME = "test_ext_alarms"; > + let alarmFired = false; > > browser.alarms.onAlarm.addListener(alarm => { > browser.test.assertEq(ALARM_NAME, alarm.name, "alarm has the correct name"); > - browser.test.notifyPass("alarms"); > + alarmFired = true; how about storing the id from `setTimeout()` below and then calling `clearTimeout()` here instead? ::: toolkit/components/extensions/test/mochitest/test_ext_alarms.html:105 (Diff revision 1) > > > add_task(function* test_alarm_fires_with_when() { > function backgroundScript() { > let ALARM_NAME = "test_ext_alarms"; > + let alarmFired = false; same comment as above ::: toolkit/components/extensions/test/mochitest/test_ext_alarms.html:210 (Diff revision 1) > }); > } > }); > browser.alarms.create(ALARM_NAME, {periodInMinutes: 0.02}); > setTimeout(() => { > - browser.test.notify("alarm fired within expected time"); > + if (count != 3) { just use `browser.test.assertEq(count, 3, "...")`
Attachment #8745089 -
Flags: review?(aswan)
Assignee | ||
Comment 6•8 years ago
|
||
Comment on attachment 8745089 [details] MozReview Request: Bug 1266824 - Intermittent test_ext_alarms.html | alarm fired within expected time, r?aswan Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48827/diff/1-2/
Attachment #8745089 -
Flags: review?(aswan)
Assignee | ||
Comment 7•8 years ago
|
||
https://reviewboard.mozilla.org/r/48827/#review45833 > how about storing the id from `setTimeout()` below and then calling `clearTimeout()` here instead? Good idea! Thanks. :) > just use `browser.test.assertEq(count, 3, "...")` The only reason I had that `if` there was to prevent the failure from being generated if the test really passed, similar to what I was doing with `alarmFired` before. I'm now using `clearTimeout` here as well, so we don't actually need this at all (nor do we need the assert).
Comment 8•8 years ago
|
||
Comment on attachment 8745089 [details] MozReview Request: Bug 1266824 - Intermittent test_ext_alarms.html | alarm fired within expected time, r?aswan https://reviewboard.mozilla.org/r/48827/#review45851 This test still has a bunch of hard-coded timers which isn't great but this patch definitely improves the situation.
Attachment #8745089 -
Flags: review?(aswan) → review+
Assignee | ||
Comment 9•8 years ago
|
||
https://reviewboard.mozilla.org/r/48827/#review45851 I'm not sure what you mean by this. Can you give an example of what would be an improvement? Maybe we can open a separate bug to cover the changes you'd like to see.
Assignee | ||
Comment 10•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=759837e7d775
Comment 11•8 years ago
|
||
https://reviewboard.mozilla.org/r/48827/#review45851 I think that in short, any use of `setTimeout()` with a hard-coded timer value is suspicious. This writeup is about something slightly narrower but I think it applies more broadly: https://developer.mozilla.org/en-US/docs/Mozilla/QA/Avoiding_intermittent_oranges#Using_magical_timeouts_to_cause_delays The twist here is that the current implementation of alarms makes them more or less equivalent to `setTimeout()` so its hard to get away from it entirely.
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 12•8 years ago
|
||
https://reviewboard.mozilla.org/r/48827/#review45851 Ah, now I know what you're talking about! This is like the idea that we should never use `sleep()` in our Python tests. I agree wholeheartedly. The issue here, as you've indicated above, is that we essentially *need* to wait because that's what the alarm code does. We could probably remove some of these `setTimeout` calls if we waited for messages instead, but there are a few downsides of this: - Any test that failed would fail because of a test timeout, which would provide a much less useful failure message. - If a test failed because an alarm did not fire, we wouldn't have an opportunity to clear the alarm, as we're doing now, although we could probably address that via some sort of tearDown type method. - We wouldn't be able to wait a specific amount of time, which is what we are doing now, although that is of less value than it seems as we're not sure exactly how long we need to wait. I don't really see that as an improvement to the tests, and I don't think we are particularly misusing `setTimeout` in these tests, but if you feel strongly about it we can open another bug to track improving these tests.
Comment 13•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/39fed34b1348
Keywords: checkin-needed
Comment 14•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/39fed34b1348
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Comment hidden (Intermittent Failures Robot) |
Comment 16•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/31459f2b72ac
status-firefox48:
--- → fixed
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•