Status
()
People
(Reporter: johnath, Assigned: johnath)
Tracking
({fixed1.9.1})
Firefox Tracking Flags
(Not tracked)
Details
Attachments
(1 attachment)
|
4.58 KB,
patch
|
Gavin
:
review+
beltzner
:
approval1.9.1+
|
Details | Diff | Splinter Review |
If possible, we should re-write those tests to use a load listener of some kind. Error pages don't fire onload, but DOMContentLoaded should work. If it doesn't, gavin also mentioned using setInterval instead, and polling for success, instead of a one-shot timer which may miss page load and create a false failure.
Blocks: 460474
| (Assignee) | ||
Updated•9 years ago
|
||
Summary: Tests for bug 400731 use timers, might be fragile. → Tests for bug 400731, 431826 use timers, are fragile.
| (Assignee) | ||
Comment 1•9 years ago
|
||
Created attachment 372610 [details] [diff] [review] Switch to DOMContentLoaded I am basically certain that at some point in the past I tried to use DOMContentLoaded listeners here and saw failures, but testing it just now locally, both tests passed with this change. Running the patch through try server now.
Attachment #372610 -
Flags: review?(gavin.sharp)
| (Assignee) | ||
Comment 2•9 years ago
|
||
Try server barfed a little the first time, but after two runs, I got greens on each platform at least once. Honestly, if DOMContentLoaded listeners work "some of the time" on error pages, that's a test that should fail, because seriously, wtf?
Updated•9 years ago
|
||
Attachment #372610 -
Flags: review?(gavin.sharp) → review+
| (Assignee) | ||
Comment 3•9 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/7c4109d88624
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
| (Assignee) | ||
Comment 4•9 years ago
|
||
Comment on attachment 372610 [details] [diff] [review] Switch to DOMContentLoaded Assuming this tests green, landing it on branch should be a zero-pain way to remove an intermittent orange. This is a test-only change.
Attachment #372610 -
Flags: approval1.9.1?
Comment 5•9 years ago
|
||
Comment on attachment 372610 [details] [diff] [review] Switch to DOMContentLoaded Test only changes don't require approvals, but since you asked so nicely ... a+!
Attachment #372610 -
Flags: approval1.9.1? → approval1.9.1+
Comment 6•9 years ago
|
||
Is this still wanted for 1.9.1?
| (Assignee) | ||
Comment 7•9 years ago
|
||
(In reply to comment #6) > Is this still wanted for 1.9.1? Opportunistically, if the tree is green and someone gets a chance, yes. It hasn't caused any oranges that I know of on trunk, so bringing it to branch might reduce the chance of the old versions causing an intermittent orange.
| (Assignee) | ||
Comment 8•9 years ago
|
||
(In reply to comment #7) > Opportunistically, if the tree is green and someone gets a chance, yes. It > hasn't caused any oranges that I know of on trunk, so bringing it to branch > might reduce the chance of the old versions causing an intermittent orange. The tree was green, and I got a chance. :) http://hg.mozilla.org/releases/mozilla-1.9.1/rev/9964b0a18b28
Keywords: fixed1.9.1
Updated•4 years ago
|
||
Component: Phishing Protection → Phishing Protection
Product: Firefox → Toolkit
You need to log in
before you can comment on or make changes to this bug.
Description
•