Closed
Bug 1361170
Opened 8 years ago
Closed 7 years ago
[in-content-old] Intermittent browser_advanced_siteData.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 :: Settings UI, defect)
Firefox
Settings UI
Tracking
()
RESOLVED
FIXED
Firefox 56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: jaws, Assigned: hemantsingh1612, Mentored)
References
Details
(Keywords: intermittent-failure, Whiteboard: [stockwell fixed:other])
Attachments
(1 file)
+++ This bug was initially created as a clone of Bug #1349051 +++
Just like what was done for bug 1349051, we should split the in-content-old version of browser_advanced_siteData.js.
The person implementing the patch for this bug can follow https://hg.mozilla.org/mozilla-central/rev/c83ec472c2f4 as a guide.
browser_advanced_siteData.js is located at /browser/preferences/in-content-old/tests/browser_advanced_siteData.js
Reporter | ||
Updated•8 years ago
|
Whiteboard: [stockwell][lang=js] → [stockwell][lang=js][good first bug]
Comment hidden (offtopic) |
Reporter | ||
Comment 3•8 years ago
|
||
(In reply to Hemant Singh Patwal [:hhhh1612] from comment #2)
> Hi I want to work on this !
Great! Please go ahead and begin working on it. I will assign the bug to you when you attach a patch.
Flags: needinfo?(jaws)
Comment hidden (Intermittent Failures Robot) |
Reporter | ||
Comment 5•7 years ago
|
||
Hi, were you still planning on working on this bug?
Flags: needinfo?(hemantsingh1612)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(hemantsingh1612)
Reporter | ||
Updated•7 years ago
|
Assignee: nobody → hemantsingh1612
Status: NEW → ASSIGNED
Reporter | ||
Comment 8•7 years ago
|
||
mozreview-review |
Comment on attachment 8870705 [details]
Bug 1361170 - Split browser_advanced_siteData.js in to two tests because it was running too long.
https://reviewboard.mozilla.org/r/142184/#review146026
Thanks, this looks good. I checked that there were no changes introduced when the test was split apart.
I will grant r+ if you update the commit message as described below.
::: commit-message-6dfa5:1
(Diff revision 2)
> +Bug 1361170 - [in-content-old] Intermittent browser_advanced_siteData.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. r?jaws
This commit message is unhelpful. Please change it to the following:
Bug 1361170 - Split browser_advanced_siteData.js in to two tests because it was running too long. r?jaws
Attachment #8870705 -
Flags: review?(jaws) → review-
Comment hidden (mozreview-request) |
Assignee | ||
Comment 10•7 years ago
|
||
We are getting linting errors, browser_advanced_siteData2.js .I am not sure about that
Reporter | ||
Comment 11•7 years ago
|
||
mozreview-review |
Comment on attachment 8870705 [details]
Bug 1361170 - Split browser_advanced_siteData.js in to two tests because it was running too long.
https://reviewboard.mozilla.org/r/142184/#review146034
Thanks!
Attachment #8870705 -
Flags: review?(jaws) → review+
Reporter | ||
Comment 12•7 years ago
|
||
(In reply to Hemant Singh Patwal [:hhhh1612] from comment #10)
> We are getting linting errors, browser_advanced_siteData2.js .I am not sure
> about that
Sorry I didn't see your message before my review. What are your linting errors?
Assignee | ||
Comment 13•7 years ago
|
||
(In reply to Jared Wein [:jaws] (behind on reviews and bugmail) (please needinfo? me) from comment #12)
> Sorry I didn't see your message before my review. What are your linting
> errors?
These are `no-undef` errors in browser_siteData2.js.
Flags: needinfo?(jaws)
Reporter | ||
Comment 14•7 years ago
|
||
Hm, I don't get those errors when I apply your patch and run it through eslint on mozilla-central tip. But when I run the test I do get errors that mockSiteDataManager, TEST_OFFLINE_URL, promiseSitesUpdated, and addPersistentStoragePerm are not defined. Those may need to be moved to head.js in the directory so that both tests can reference them.
Flags: needinfo?(jaws)
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Reporter | ||
Comment 17•7 years ago
|
||
Hi Hemant, are you able to rebase your patch and fix the eslint errors?
Flags: needinfo?(hemantsingh1612)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 20•7 years ago
|
||
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #14)
> Hm, I don't get those errors when I apply your patch and run it through
> eslint on mozilla-central tip. But when I run the test I do get errors that
> mockSiteDataManager, TEST_OFFLINE_URL, promiseSitesUpdated, and
> addPersistentStoragePerm are not defined. Those may need to be moved to
> head.js in the directory so that both tests can reference them.
There are other like assertSitesListed,REMOVE_DIALOG_URL, with no-undef .Should we move all of them in head.js. But this way It will mean a lot to be moved in head.js.
Flags: needinfo?(hemantsingh1612)
Reporter | ||
Comment 21•7 years ago
|
||
Yes, please move them all to head.js. Since you are splitting the test it is expected that the shared references will need to be moved.
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(jaws)
Comment hidden (Intermittent Failures Robot) |
Reporter | ||
Comment 24•7 years ago
|
||
mozreview-review |
Comment on attachment 8870705 [details]
Bug 1361170 - Split browser_advanced_siteData.js in to two tests because it was running too long.
https://reviewboard.mozilla.org/r/142184/#review152982
This looks good but it will need to be rebased since there have been some changes to the folder names of the preferences. Sorry I didn't see this earlier. Thanks for the needinfo, I didn't notice that a new patch was uploaded and didn't get notified because the patch already had r+.
Reporter | ||
Updated•7 years ago
|
Flags: needinfo?(jaws)
Comment hidden (Intermittent Failures Robot) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 27•7 years ago
|
||
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #24)
> https://reviewboard.mozilla.org/r/142184/#review152982
>
> This looks good but it will need to be rebased since there have been some
> changes to the folder names of the preferences. Sorry I didn't see this
> earlier. Thanks for the needinfo, I didn't notice that a new patch was
> uploaded and didn't get notified because the patch already had r+.
Hi I have rebased.Please have a look .
Flags: needinfo?(jaws)
Comment hidden (mozreview-request) |
Reporter | ||
Comment 29•7 years ago
|
||
(In reply to Hemant Singh Patwal [:hhhh1612] from comment #27)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #24)
>
> > https://reviewboard.mozilla.org/r/142184/#review152982
> >
> > This looks good but it will need to be rebased since there have been some
> > changes to the folder names of the preferences. Sorry I didn't see this
> > earlier. Thanks for the needinfo, I didn't notice that a new patch was
> > uploaded and didn't get notified because the patch already had r+.
>
> Hi I have rebased.Please have a look .
Thanks, I just pushed your patch to tryserver to make sure that all tests pass before we land it. Test results will show up at https://treeherder.mozilla.org/#/jobs?repo=try&revision=1f0da980a0a1
Flags: needinfo?(jaws)
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment 32•7 years ago
|
||
I am excited to see the patch on this bug and work being done!
this has really picked up in frequency, the try push should use linux debug browser-chrome as that is where the failures are most seen.
Can we work to get this tested/reviewed/landed this week given the fact that this has really increased in frequency?
Whiteboard: [stockwell][lang=js][good first bug] → [stockwell needswork][lang=js][good first bug]
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Reporter | ||
Comment 35•7 years ago
|
||
The test failed on the tryserver with the following error message:
"This test didn't call add_task, nor did it define a generatorTest() function, nor did it define a test() function, so we don't know how to run it."
Did you run the test locally? Please make sure to run the test locally and ask questions here if you have trouble getting the test to work.
To run the test, use the command `./mach test path/to/test` where "path/to/test" is replaced by the path to the test that you want to run.
Flags: needinfo?(hemantsingh1612)
Assignee | ||
Comment 36•7 years ago
|
||
Hi
Trying to run test|./mach test browser\components\preferences\in-content\tests\browser_advanced_siteData.js| gives :
$ ./mach test browser\components\preferences\in-content\tests\browser_advanced_siteData.js
UNKNOWN TEST: browsercomponentspreferencesin-contenttestsbrowser_advanced_siteData.js
I was unable to find tests from the given argument(s).
You should specify a test directory, filename, test suite name, or
abbreviation. If no arguments are given, there must be local file
changes and corresponding IMPACTED_TESTS annotations in moz.build
files relevant to those files.
It's possible my little brain doesn't know about the type of test you are
trying to execute. If you suspect this, please request support by filing
a bug at
https://bugzilla.mozilla.org/enter_bug.cgi?product=Testing&component=General.
Flags: needinfo?(hemantsingh1612) → needinfo?(jaws)
Comment 37•7 years ago
|
||
it looks like you are on windows, I am on windows 10 and use the mozilla-build environment to build/test firefox- I can reproduce your problem, but there is a fix if you use / instead of \ in the path:
|./mach test browser/components/preferences/in-content/tests/browser_advanced_siteData.js|
We have had 2+ weeks of high failure rates, lets please make this a priority- Wednesday is my scheduled day to disable tests that are too problematic.
Flags: needinfo?(jaws)
Comment 38•7 years ago
|
||
checking in here, were you able to run this locally?
Flags: needinfo?(hemantsingh1612)
Assignee | ||
Comment 39•7 years ago
|
||
(In reply to Joel Maher ( :jmaher) from comment #38)
> checking in here, were you able to run this locally?
Yep, initially test runs fine,but after applying this patch I am getting the same error message mentioned in comment 35.
Flags: needinfo?(hemantsingh1612)
Reporter | ||
Comment 40•7 years ago
|
||
mozreview-review |
Comment on attachment 8870705 [details]
Bug 1361170 - Split browser_advanced_siteData.js in to two tests because it was running too long.
https://reviewboard.mozilla.org/r/142184/#review158346
It looks like you removed too much from browser_siteData.js. You should compare your changes here to the changes from https://hg.mozilla.org/mozilla-central/rev/c83ec472c2f4
Attachment #8870705 -
Flags: review+ → review-
Comment hidden (mozreview-request) |
Assignee | ||
Comment 42•7 years ago
|
||
I have updated patch everything fine now. :)
Flags: needinfo?(jaws)
Reporter | ||
Comment 43•7 years ago
|
||
Try push with latest patch,
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0bc89abaad5cdd7c379c1b6ff4a2cff434985d29
Flags: needinfo?(jaws)
Reporter | ||
Comment 44•7 years ago
|
||
mozreview-review |
Comment on attachment 8870705 [details]
Bug 1361170 - Split browser_advanced_siteData.js in to two tests because it was running too long.
https://reviewboard.mozilla.org/r/142184/#review158554
The patch looks good and I don't see any related failures on tryserver. Thanks!
Attachment #8870705 -
Flags: review?(jaws) → review+
Comment 45•7 years ago
|
||
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/573325888d29
Split browser_advanced_siteData.js in to two tests because it was running too long. r=jaws
Comment hidden (Intermittent Failures Robot) |
Comment 47•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Updated•7 years ago
|
Whiteboard: [stockwell needswork][lang=js][good first bug] → [stockwell fixed:other]
Comment hidden (Intermittent Failures Robot) |
You need to log in
before you can comment on or make changes to this bug.
Description
•