Closed Bug 1273871 Opened 8 years ago Closed 8 years ago

Intermittent passwordmgr/test/browser/browser_capture_doorhanger.js | This test exceeded the timeout threshold. It should be rewritten or split up. If that's not possible, use requestLongerTimeout(N), but only as a last resort.

Categories

(Toolkit :: Password Manager, defect, P3)

Product:
Toolkit
Component:
Password Manager
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox49 --- wontfix
firefox50 --- wontfix
firefox51 --- fixed
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: aryx, Assigned: evanxd)

References

Details

(Keywords: intermittent-failure)

Attachments

(1 file)

Bug 1273871 - Split up browser_capture_doorhanger.js test for avoiding the timeout issue.
58 bytes, text/x-review-board-request
MattN
: review+
Details 
https://treeherder.mozilla.org/logviewer.html#?job_id=28100466&repo=mozilla-inbound

06:57:29     INFO -  466 INFO TEST-PASS | toolkit/components/passwordmgr/test/browser/browser_capture_doorhanger.js | Should only have 1 login -
06:57:29     INFO -  467 INFO TEST-PASS | toolkit/components/passwordmgr/test/browser/browser_capture_doorhanger.js | Check the username unchanged -
06:57:29     INFO -  468 INFO TEST-PASS | toolkit/components/passwordmgr/test/browser/browser_capture_doorhanger.js | Check the password unchanged -
06:57:29     INFO -  469 INFO TEST-PASS | toolkit/components/passwordmgr/test/browser/browser_capture_doorhanger.js | Check times used incremented -
06:57:29     INFO -  470 INFO Leaving test bound test_recipeCaptureFields_ExistingLogin
06:57:29     INFO -  471 INFO TEST-UNEXPECTED-FAIL | toolkit/components/passwordmgr/test/browser/browser_capture_doorhanger.js | This test exceeded the timeout threshold. It should be rewritten or split up. If that's not possible, use requestLongerTimeout(N), but only as a last resort. -
See Also: → 1277105
Bulk assigning P3 to all open intermittent bugs without a priority set in Firefox components per bug 1298978.
Priority: -- → P3
Evan, I see you've been looking at this test recently. This particular failure is very frequent, is there any chance you could take a look?
status-firefox49: affected → wontfix
status-firefox50: --- → affected
status-firefox51: --- → affected
status-firefox52: --- → affected
Flags: needinfo?(evan)
Summary: Intermittent /passwordmgr/test/browser/browser_capture_doorhanger.js | This test exceeded the timeout threshold. It should be rewritten or split up. If that's not possible, use requestLongerTimeout(N), but only as a last resort. - → Intermittent passwordmgr/test/browser/browser_capture_doorhanger.js | This test exceeded the timeout threshold. It should be rewritten or split up. If that's not possible, use requestLongerTimeout(N), but only as a last resort.
Ryan,

Sure, let me do it.
Assignee: nobody → evan
Flags: needinfo?(evan)
Pushed a try: Split up the browser_capture_doorhanger.js test to avoid the timeout issue.

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=971af694d34c
Hi Matthew,

You might haven't seen the review request yet.

Could you help review the patch? Thanks.
Flags: needinfo?(MattN+bmo)
Evan, what makes you think that the test needs to be split up? This error message is misleading and suggests splitting up tests even when it's not necessary. Was the test failing always at the same place or different places?

Also, when splitting files, please use an `hg copy` to preserve the blame and to make it easier for reviewers to know what changed.
Flags: needinfo?(MattN+bmo)
Comment on attachment 8797029 [details]
Bug 1273871 - Split up browser_capture_doorhanger.js test for avoiding the timeout issue.

See comment 32
Attachment #8797029 - Flags: review?(MattN+bmo)
(In reply to Matthew N. [:MattN] (away Nov. 3–4) from comment #32)
> This error
> message is misleading and suggests splitting up tests even when it's not
> necessary. 

Why do you think it's misleading? it just means the test took more than 45s to complete, and as such either there's a perf bug, or the test can be made faster, or it should be split into multiple tests.
(In reply to Marco Bonardo [::mak] from comment #37)
> (In reply to Matthew N. [:MattN] (away Nov. 3–4) from comment #32)
> > This error
> > message is misleading and suggests splitting up tests even when it's not
> > necessary. 
> 
> Why do you think it's misleading? it just means the test took more than 45s
> to complete, and as such either there's a perf bug, or the test can be made
> faster, or it should be split into multiple tests.

Because the most common case I see (which is a case you didn't mention) is that a test is yielding on a Promise which never resolves. That can be caused by a regression or a bug in the test with how it resolves the Promise e.g. not creating the Promise at the right time.
(In reply to Matthew N. [:MattN] (away Nov. 3–4) from comment #39)
> Because the most common case I see (which is a case you didn't mention) is
> that a test is yielding on a Promise which never resolves. That can be
> caused by a regression or a bug in the test with how it resolves the Promise
> e.g. not creating the Promise at the right time.

Nope, this error only happens when a test CAN finish, but it takes more than 45 seconds.
If the test does not finish it timeouts normally with a test timeout error.
So the message is perfectly correct, the test finished but it took more than 45s, so it's too slow.

What we do when the test goes over the 45s timeout is giving it more time, for up to 10 times. If after that time it still didn't finish, we timeout.
The reason to do this is that timing out b-c tests very often causes a waterfall of failures in all the next tests, if instead we leave them run to completion, the next tests will be fine. Though we still want to know that the test took too much time and needs maintenance, thus this error.
we could edit the text as "This test _finished but_ exceeded the timeout threshold..." but then a bunch of automatically starred bugs would need to be edited.
Based on Comment 40, I think we should split up the test, right?
Flags: needinfo?(MattN+bmo)
Whiteboard: [test disabled]
Pushed by philringnalda@gmail.com:
https://hg.mozilla.org/mozilla-central/rev/a459d3383943
disable browser_capture_doorhanger.js for constant failures, a=nnoyance
(In reply to Marco Bonardo [::mak] from comment #40)
> (In reply to Matthew N. [:MattN] (away Nov. 3–4) from comment #39)
> > Because the most common case I see (which is a case you didn't mention) is
> > that a test is yielding on a Promise which never resolves. That can be
> > caused by a regression or a bug in the test with how it resolves the Promise
> > e.g. not creating the Promise at the right time.
> 
> Nope, this error only happens when a test CAN finish, but it takes more than
> 45 seconds.
> If the test does not finish it timeouts normally with a test timeout error.
> So the message is perfectly correct, the test finished but it took more than
> 45s, so it's too slow.

Hmm… maybe this improved at some point? I remember years ago finding that the instructions to split up were misleading and leading to over-eagerly splitting tests.
Flags: needinfo?(MattN+bmo)
Comment on attachment 8797029 [details]
Bug 1273871 - Split up browser_capture_doorhanger.js test for avoiding the timeout issue.

https://reviewboard.mozilla.org/r/82672/#review94428

I don't know if this is sufficient since the try push didn't include linux debug (the most common failure platform) and didn't include re-triggers to see if it will still fail intermittently but I'm fine with the patch.

::: toolkit/components/passwordmgr/test/browser/browser.ini:17
(Diff revision 1)
>    insecure_test.html
>    insecure_test_subframe.html
>    multiple_forms.html
>    streamConverter_content.sjs
>  
>  [browser_capture_doorhanger.js]

Please include a backout of https://hg.mozilla.org/mozilla-central/rev/a459d3383943 in this commit now to re-enable the original test file.

::: toolkit/components/passwordmgr/test/browser/browser.ini:31
(Diff revision 1)
>    subtst_notifications_6.html
>    subtst_notifications_8.html
>    subtst_notifications_9.html
>    subtst_notifications_10.html
>    subtst_notifications_change_p.html
> +[browser_capture_doorhanger_https.js]

Nit: "browser_capture_doorhanger_httpsUpgrade.js" since it's focused on the httpsUpgrade cases.

::: toolkit/components/passwordmgr/test/browser/browser_capture_doorhanger.js
(Diff revision 1)
> -add_task(function* test_httpsUpgradeCaptureFields_noChange() {
> -  info("Check that we don't prompt to remember when capturing an upgraded login with no change");
> -  Services.logins.addLogin(login1);
> -  // Sanity check the HTTP login exists.
> -  let logins = Services.logins.getAllLogins();
> -  is(logins.length, 1, "Should have the HTTP login");

You still didn't address:
(Quoting Matthew N. [:MattN] from comment #32)
> Also, when splitting files, please use an `hg copy` to preserve the blame
> and to make it easier for reviewers to know what changed.

You can use: `hg cp --after toolkit/components/passwordmgr/test/browser/browser_capture_doorhanger.js toolkit/components/passwordmgr/test/browser/browser_capture_doorhanger_https.js` if you have the files modified in the working directory.

::: toolkit/components/passwordmgr/test/browser/browser_capture_doorhanger_https.js:2
(Diff revision 1)
> +/*
> + * Test capture popup notifications generated by HTTPS logins

Also clarify this comment to mention that it's testing HTTPS upgades:
Test capture popup notifications with HTTPS upgrades

Thanks
Attachment #8797029 - Flags: review+
Updated patch for comments and sent a try push[1]. If tests are all good, let's re-enable the tests.

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b727cadbfe80
Re-trigger the tests[1] for multiple times. If tests are all good, let's re-enable it.

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b727cadbfe80&selectedJob=31659363
Hi Matthew,

I re-triggered the test for over 30 times and the tests are all passed already. I think we could re-enable the test again.

Could you help land the patch on MozReviewBoard?(I don't have the permission) Thanks.
Flags: needinfo?(MattN+bmo)
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/autoland/rev/04ba25cbcf8a
Split up browser_capture_doorhanger.js test for avoiding the timeout issue. r=MattN
Done
Flags: needinfo?(MattN+bmo)
Thanks Matthew.
backed out for perma failures like https://treeherder.mozilla.org/logviewer.html#?job_id=7087855&repo=autoland
Flags: needinfo?(evan)
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cb894821cf44
Backed out changeset 04ba25cbcf8a for bc5 failures
> INFO - TEST-UNEXPECTED-FAIL | toolkit/components/passwordmgr/test/browser/browser_capture_doorhanger.js | This test exceeded the timeout threshold. It should be rewritten or split up. If that's not possible, use requestLongerTimeout(N), but only as a last resort.

Timeout issue again. Looks like we need to split more tests in browser_capture_doorhanger.js.

What do you think, Matthew?
Flags: needinfo?(evan) → needinfo?(MattN+bmo)
Or just reland with the requestLongerTimeout back: https://hg.mozilla.org/integration/autoland/rev/04ba25cbcf8a#l2.21
Flags: needinfo?(MattN+bmo)
Hi Matthew,

I've updated the patch[1] to add `requestLongerTimeout` back and sent a try push.

Could you help re-land it? Thanks.

[1]: https://reviewboard.mozilla.org/r/82672/diff/3#index_header
Hi Evan, you can set the checkin-needed keyword on the bug in the future as another option. I queued it for landing.
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/autoland/rev/a06c5b8741d7
Split up browser_capture_doorhanger.js test for avoiding the timeout issue. r=MattN
(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #69)
> Hi Evan, you can set the checkin-needed keyword on the bug in the future as
> another option. I queued it for landing.

Sure, got it. Thanks for landing.
https://hg.mozilla.org/mozilla-central/rev/a06c5b8741d7
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
https://hg.mozilla.org/releases/mozilla-aurora/rev/8e1c11eedc84
status-firefox52: affected → fixed
Flags: in-testsuite+
Whiteboard: [test disabled]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: