Intermittent [tier2] browser/components/urlbar/tests/browser/browser_search_tips.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
(Firefox :: Address Bar, defect, P3)
Tracking
()
People
(Reporter: intermittent-bug-filer, Assigned: mak)
References
(Regression)
Details
(Keywords: intermittent-failure, regression)
Attachments
(1 file, 2 obsolete files)
(deleted),
text/x-phabricator-request
|
Details |
Filed by: dvarga [at] mozilla.com
Parsed log: https://treeherder.mozilla.org/logviewer.html#?job_id=287655807&repo=mozilla-central
Full log: https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/B4Ws0baYQIieQWq6n5sBMw/runs/0/artifacts/public/logs/live_backing.log
[task 2020-02-05T23:29:24.761Z] 23:29:24 INFO - TEST-PASS | browser/components/urlbar/tests/browser/browser_search_tips.js | {"_name":"Google","_shortName":"google-b-1-d","_loadPath":"[other]addEngineWithDetails:google@search.mozilla.org","description":"Google Search","__searchForm":"https://www.google.com/search?client=firefox-b-1-d&q={searchTerms}","_iconURL":"moz-extension://37b45f1f-51a7-4050-b13a-da279933d1c6/favicon.ico","_iconMapObj":{"{}":"moz-extension://37b45f1f-51a7-4050-b13a-da279933d1c6/favicon.ico"},"_metaData":{"alias":null,"order":1},"_urls":[{"params":[{"condition":"pref","mozparam":true,"name":"channel","pref":"google_channel_us"},{"name":"client","value":"firefox-b-1-d"},{"name":"q","value":"{searchTerms}"}],"rels":[],"resultDomain":"www.google.com","template":"https://www.google.com/search"},{"params":[],"rels":[],"resultDomain":"www.google.com","template":"https://www.google.com/complete/search?client=firefox&q={searchTerms}","type":"application/x-suggestions+json"}],"_isBuiltin":true,"queryCharset":"UTF-8","extensionID":"google@search.mozilla.org","extensionLocale":"b-1-d"} == true -
[task 2020-02-05T23:29:24.762Z] 23:29:24 INFO - Buffered messages finished
[task 2020-02-05T23:29:24.762Z] 23:29:24 INFO - TEST-UNEXPECTED-FAIL | browser/components/urlbar/tests/browser/browser_search_tips.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. -
[task 2020-02-05T23:29:24.762Z] 23:29:24 INFO - GECKO(7289) | MEMORY STAT | vsize 3570MB | residentFast 631MB | heapAllocated 157MB
Updated•5 years ago
|
Updated•5 years ago
|
Comment hidden (Intermittent Failures Robot) |
Updated•5 years ago
|
Comment 2•5 years ago
|
||
Comment 3•5 years ago
|
||
Depends on D62205
Comment 4•5 years ago
|
||
Comment 5•5 years ago
|
||
I'll be on PTO next week, but my patch here is pretty firmly blocked by bug 1612903. If this failure becomes an issue in the meantime, we could reduce the timeout in checkTip
when waiting for a tip to not appear. Since we don't use SHOW_TIP_DELAY_MS
anymore in the Search Tip provider, we could reduce that wait from 600ms to something much shorter like 100ms. This would save a significant amount of time on the test since that timeout is used often.
Comment hidden (Intermittent Failures Robot) |
Comment 7•5 years ago
|
||
(In reply to Harry Twyford [:harry] [PTO 2/17-21] from comment #5)
I'll be on PTO next week, but my patch here is pretty firmly blocked by bug 1612903. If this failure becomes an issue in the meantime, we could reduce the timeout in
checkTip
when waiting for a tip to not appear. Since we don't useSHOW_TIP_DELAY_MS
anymore in the Search Tip provider, we could reduce that wait from 600ms to something much shorter like 100ms. This would save a significant amount of time on the test since that timeout is used often.
It seems like we should do this anyway, or we pointlessly wait making these tests slower without a real reason.
If we don't need to wait anymore, why not removing the timeout altogether instead of just reducing to 100 ms?
Updated•5 years ago
|
Assignee | ||
Comment 8•5 years ago
|
||
(In reply to Marco Castelluccio [:marco] from comment #7)
If we don't need to wait anymore, why not removing the timeout altogether instead of just reducing to 100 ms?
Most of the urlbar code and handling is async and depends on I/O and network, when some things are supposed to NOT happen there's nothing to listen for, thus to stay on the safe side it's better to check after a timeout. I think 100ms would be an acceptable compromise.
I must note though that it would only save 5s, the timeout is at 45s and the failure above takes 48s, so we'd be barely in, that means the refactoring is still necessary longer term.
I will prepare a patch to reduce the timeout, and investigate bug 1612903 in the meanwhile.
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 9•5 years ago
|
||
Comment 10•5 years ago
|
||
Comment 11•5 years ago
|
||
bugherder |
Assignee | ||
Comment 12•5 years ago
|
||
To avoid confusion I'm going to clone this bug to work on moving/splitting tests, regardless harry's patches are bitrotted and I'd prefer a different tests structure.
Since we landed a fix here, let's close this and file a new one for that work.
That will also help tracking uplifts.
Updated•5 years ago
|
Updated•5 years ago
|
Assignee | ||
Comment 13•5 years ago
|
||
Comment on attachment 9127328 [details]
Bug 1613549 - Reduce the timeout in browser_search_tips.js because the original delay has been removed. r=adw
Beta/Release Uplift Approval Request
- User impact if declined: test only change
- Is this code covered by automated tests?: Yes
- Has the fix been verified in Nightly?: No
- Needs manual test from QE?: No
- If yes, steps to reproduce: test only change
- List of other uplifts needed: None
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): test only change
- String changes made/needed:
Updated•5 years ago
|
Comment 14•5 years ago
|
||
test-only changes don't need approval to land
Comment 15•5 years ago
|
||
uplift |
Updated•5 years ago
|
Comment hidden (Intermittent Failures Robot) |
Description
•