Closed Bug 1396184 Opened 7 years ago Closed 7 years ago

Intermittent uriloader/exthandler/tests/mochitest/browser_ext_helper_pb.js | Should have at least 1 download in ALL mode - 0 == 1

Categories

(Firefox :: File Handling, defect, P3)

Product:
Firefox
Component:
File Handling
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 58
Tracking Flags:
Tracking Status
firefox57 --- disabled
firefox58 --- disabled

People

(Reporter: intermittent-bug-filer, Assigned: Paolo)

References

Details

(Keywords: intermittent-failure, Whiteboard: [stockwell disabled])

Attachments

(1 file)

Bug 1396184 - Rewrite the test for bug 1367581.
59 bytes, text/x-review-board-request
mak
: review+
Details 
the failure rate continues to be fairly high:
https://brasstacks.mozilla.com/orangefactor/index.html?display=Bug&bugid=1396184

this is a mix of all operating systems and configurations.

this started on September 1st, not sure if this is a bug in firefox or the test.

:paolo, I see you are the triage owner for Firefox::File Handling, can you help set a priority on this failure and find an engineer to look into it when appropriate?
Flags: needinfo?(paolo.mozmail)
Whiteboard: [stockwell needswork]
:paolo, I haven't heard from you in 18 days as a triage owner, can you help us figure out a path for fixing this intermittent as it has 5 weeks now of high frequency failures?
Hi Joel, sorry, I should have commented on this earlier! I took a look at the code shortly after the original needinfo, and the reason the test is failing is that the method used by this test and a few others in the same folder to wait for the application chooser dialog to be displayed is incorrect. The test has to be rewritten.

As the triage owner I can help with setting the priority, but I can't help with finding an engineer to look into it. There is no team dedicated to this component so all the bugs get low priorities typically. I kept the needinfo on me to rewrite the test myself - which I hoped would've happened earlier - and I think this maps to P3.
Priority: P5 → P3
Thanks :Paolo, this sounds logical, let me know if you need more information!
Pushed by jmaher@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/488072ed69fb
Disable uriloader/exthandler/tests/mochitest/browser_ext_helper_pb.js on OSX for frequent failures. r=me, a=test-only
I disabled this for high frequency of failures, please remember to re-enable this while working on this.
Keywords: leave-open
Whiteboard: [stockwell disable-recommended] → [stockwell disabled]
Flags: needinfo?(paolo.mozmail)
I'm actually working on this right now :-)
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Blocks: 1411979
I've checked that we don't need to display the dialog in order to test the code that was changed in bug 1367581.
For reference, the original test was added in bug 1370787.

The patch here adds another copy of the getTempFile function. I filed bug 1411979 to get rid of this and several other duplicates we had to make. This depends on a better way to access test global functions in test-only modules.
Depends on: 1370787
Comment on attachment 8922341 [details]
Bug 1396184 - Rewrite the test for bug 1367581.

https://reviewboard.mozilla.org/r/193402/#review198634

I'd suggest a ./mach try with --artifact --rebuild 10 uriloader/exthandler/tests/mochitest/ in case you didn't yet.

::: uriloader/exthandler/tests/mochitest/browser_download_privatebrowsing.js:113
(Diff revision 1)
> +    // Clean up after checking that there are no new public downloads either.
> +    let publicDownloads = await publicList.getAll();
> +    Assert.equal(publicDownloads.length, 0);
> +    await privateList.removeFinished();
> +  });
> +  await BrowserTestUtils.closeWindow(win);

You could do this in the above registerCleanupFunction. You probably don't even need to use withNewTab, since it's a tab loaded in a new window you are closing.
The code may end up being more readable using openNewForegroundTab and just closing the window in rCF.
Attachment #8922341 - Flags: review?(mak77) → review+
(In reply to Marco Bonardo [::mak] from comment #23)
> You could do this in the above registerCleanupFunction

That would work but not be a good pattern, because it would run at the end of all tests if we add more cases. Even though, it's better to use best practices in case they get copied and pasted.

Actually, the same goes for the mock object, it's probably better to move it to its own setup function to make it clear that it applies throughout the file.

> The code may end up being more readable using openNewForegroundTab

I'll do this.
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/1ad08656cab6
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: