Closed Bug 1613549 Opened 2 years ago Closed 2 years ago

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)

defect
Points:
2

Tracking

()

RESOLVED FIXED
Firefox 75
Iteration:
75.1 - Feb 10 - Feb 23
Tracking Status
firefox74 --- fixed
firefox75 --- fixed

People

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

References

(Regression)

Details

(Keywords: intermittent-failure)

Attachments

(1 file, 2 obsolete files)

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
Regressed by: 1606909
Blocks: 1606904
Summary: Perma [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. - → 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. -
Assignee: nobody → htwyford
Status: NEW → ASSIGNED
Iteration: --- → 75.1 - Feb 10 - Feb 23
Points: --- → 2

Depends on D62205

Depends on: 1612903

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.

(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 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.

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?

Flags: needinfo?(htwyford)

(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.

Flags: needinfo?(htwyford)
Keywords: leave-open
Priority: P5 → P3
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/ba373603f390
Reduce the timeout in browser_search_tips.js because the original delay has been removed. r=adw

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.

Assignee: htwyford → mak
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Keywords: leave-open
Resolution: --- → FIXED
Attachment #9125418 - Attachment is obsolete: true
Attachment #9125419 - Attachment is obsolete: true

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:
Attachment #9127328 - Flags: approval-mozilla-beta?
Blocks: 1616631
Attachment #9127328 - Flags: approval-mozilla-beta?

test-only changes don't need approval to land

Whiteboard: [checkin-needed-beta]
Whiteboard: [checkin-needed-beta]
You need to log in before you can comment on or make changes to this bug.