Closed Bug 1361170 Opened 7 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)

Product:
Firefox
Component:
Settings UI
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: jaws, Assigned: hemantsingh1612, Mentored)

References

Details

(Keywords: intermittent-failure, Whiteboard: [stockwell fixed:other])

Attachments

(1 file)

Bug 1361170 - Split browser_advanced_siteData.js in to two tests because it was running too long.
59 bytes, text/x-review-board-request
jaws
: review+
Details 
+++ 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
Whiteboard: [stockwell][lang=js] → [stockwell][lang=js][good first bug]
Hi I want to work on this !
Flags: needinfo?(jaws)
(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)
Hi, were you still planning on working on this bug?
Flags: needinfo?(hemantsingh1612)
Flags: needinfo?(hemantsingh1612)
Assignee: nobody → hemantsingh1612
Status: NEW → ASSIGNED
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-
We are getting linting errors, browser_advanced_siteData2.js .I am not sure about that
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+
(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?
(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)
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)
Hi Hemant, are you able to rebase your patch and fix the eslint errors?
Flags: needinfo?(hemantsingh1612)
(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)
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.
Flags: needinfo?(jaws)
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+.
Flags: needinfo?(jaws)
(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)
(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)
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]
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)
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)
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)
checking in here, were you able to run this locally?
Flags: needinfo?(hemantsingh1612)
(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)
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-
I have updated patch everything fine now. :)
Flags: needinfo?(jaws)
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+
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
https://hg.mozilla.org/mozilla-central/rev/573325888d29
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Whiteboard: [stockwell needswork][lang=js][good first bug] → [stockwell fixed:other]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: