Intermittent browser_rules_add-rule_01.js | This test exceeded the timeout threshold. It should be rewritten or split up.

RESOLVED FIXED in Firefox 46

Status

()

Firefox
Developer Tools: CSS Rules Inspector
P2
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: philor, Assigned: jdescottes)

Tracking

({intermittent-failure})

unspecified
Firefox 47
intermittent-failure
Points:
---

Firefox Tracking Flags

(firefox45 unaffected, firefox46 fixed, firefox47 fixed, firefox49 fixed, firefox-esr45 unaffected)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(6 attachments, 1 obsolete attachment)

Comment 1

2 years ago
24 automation job failures were associated with this bug in the last 7 days.

Repository breakdown:
* mozilla-inbound: 11
* mozilla-central: 6
* fx-team: 4
* try: 3

Platform breakdown:
* linux32: 23
* linux64: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1237885&startday=2016-01-25&endday=2016-01-31&tree=all

Comment 2

2 years ago
53 automation job failures were associated with this bug in the last 7 days.

Repository breakdown:
* mozilla-inbound: 24
* fx-team: 15
* try: 7
* mozilla-central: 5
* mozilla-aurora: 2

Platform breakdown:
* linux32: 51
* linux64: 2

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1237885&startday=2016-02-01&endday=2016-02-07&tree=all

Comment 3

2 years ago
This seems to have gotten worse around Jan 27.  Ryan, any ideas?
Flags: needinfo?(jryans)
:pbro owns this area, let's see what he says.
Component: Developer Tools: Font Inspector → Developer Tools: CSS Rules Inspector
Flags: needinfo?(jryans) → needinfo?(pbrosset)
This test can be pretty easily split in 2. It tests adding various CSS rules and does this, for every test case, twice. First by using the "add rule" button, and a second time by using the context menu.
Now, as to why it would take longer to run now, I'm not sure. I have fixed several bugs around Jan 27 that aimed at getting rid of unhandled promise rejections in rule-view tests. Most of the fixes were about having the tests wait for the right events, and so that could explain tests taking a bit longer to run.
Having said this, I don't think I touched this test in particular.
This could have something to do with changes I made in head.js.

So, anyway, not sure. But I do think we should split this test.
Flags: needinfo?(pbrosset)
Assignee: nobody → jdescottes
The current mochitest timeout is at 45 seconds [1]. This test, when successful, takes more than 40 seconds, so it's not surprising we have a high failure rate. Just stating this to confirm splitting should fix the issue.

[1] https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/browser-test.js#3
Created attachment 8718811 [details] [diff] [review]
bug1237885.v1.patch

try : https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c516b97e1c5

Bug 1237885 - fix add-rules_01 intermittent by splitting in two tests;r=gl

Intermittent test add-rules_01 has been split in two test. The original test
was checking trying to create a set of rules first using the "add rule" button
and then using the context menu.

add-rules_01 only tests using the "add rule" button now. The context-menu tests
have been moved to add-rules_05. Both tests are well under 45 seconds on Linux
debug.
Attachment #8718811 - Flags: review?(gl)
Status: NEW → ASSIGNED

Comment 8

2 years ago
66 automation job failures were associated with this bug in the last 7 days.

Repository breakdown:
* mozilla-inbound: 34
* fx-team: 13
* try: 12
* mozilla-central: 7

Platform breakdown:
* linux32: 59
* linux64: 7

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1237885&startday=2016-02-08&endday=2016-02-14&tree=all
Comment on attachment 8718811 [details] [diff] [review]
bug1237885.v1.patch

Review of attachment 8718811 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/inspector/rules/test/browser_rules_add-rule_01.js
@@ +38,5 @@
>  
>    for (let data of TEST_DATA) {
> +    let {node, expected} = data;
> +    yield selectNode(node, inspector);
> +    yield addNewRule(inspector, view, "context-menu");

This should be addNewRule(inspector, view)

::: devtools/client/inspector/rules/test/browser_rules_add-rule_02.js
@@ +21,5 @@
>    yield addTab("data:text/html;charset=utf-8," + encodeURIComponent(TEST_URI));
>    let {inspector, view} = yield openRuleView();
>    yield selectNode("#testid", inspector);
>  
> +  yield addNewRule(inspector, view, "button");

This should be addNewRule(inspector, view)

::: devtools/client/inspector/rules/test/browser_rules_add-rule_03.js
@@ +21,5 @@
>    yield addTab("data:text/html;charset=utf-8," + encodeURIComponent(TEST_URI));
>    let {inspector, view} = yield openRuleView();
>    yield selectNode("#testid", inspector);
>  
> +  yield addNewRule(inspector, view, "button");

This should be addNewRule(inspector, view)
Attachment #8718811 - Flags: review?(gl) → review+
Created attachment 8720698 [details] [diff] [review]
bug1237885.v2.patch (rebased and applied review comments)

Thanks for the review, and good catch with the useless argument names!

Carry over r+.
Attachment #8718811 - Attachment is obsolete: true
Attachment #8720698 - Flags: review+
Can you push attachment 8720698 [details] [diff] [review] ?
Keywords: checkin-needed

Comment 13

2 years ago
15 automation job failures were associated with this bug yesterday.

Repository breakdown:
* mozilla-inbound: 10
* try: 4
* fx-team: 1

Platform breakdown:
* linux32: 12
* linux64: 3

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1237885&startday=2016-02-18&endday=2016-02-18&tree=all

Comment 14

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/05f74b15d9cf
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
https://hg.mozilla.org/releases/mozilla-aurora/rev/25d7bd3a8c19
status-firefox45: --- → unaffected
status-firefox46: --- → fixed
status-firefox-esr45: --- → unaffected
Blocks: 1242986
Thanks to RyanVM, we can now run tests on try on linux debug e10s by applying the patch attachment 8756417 [details] [diff] [review] (from bug 1242986) and pushing with |try: -b d -p linux64 -u mochitest-e10s-devtools-chrome|
This is really frequent on Linux debug e10s.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 18

2 years ago
19 automation job failures were associated with this bug yesterday.

Repository breakdown:
* mozilla-inbound: 17
* ash: 2

Platform breakdown:
* linux64: 18
* linux32: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1237885&startday=2016-05-25&endday=2016-05-25&tree=all
2 logs where the test was seen failing: 
https://public-artifacts.taskcluster.net/X1lHc9A4S9mDPvR4VX1iGg/0/public/logs/live_backing.log
https://public-artifacts.taskcluster.net/VtBk1DpYQAKezsFV9A6p_A/0/public/logs/live_backing.log
The test took ~50 seconds.

Looking at the test, we could potentially still make it smaller:
- stop refreshing the whole page's HTML (basically we reset document.body.innerHTML to its original value)
- make a new tests for testing adding a property to a new rule, instead of adding a property to each and every new rule in this test.

This should help make the test go below the threshold on linux e10s debug.
Inspector bug triage (filter on CLIMBING SHOES).
Priority: -- → P2
I have worked on bug 1275083 previously, and it seems very similar, so I'll upload a patch here as well.
Created attachment 8757456 [details]
MozReview Request: Bug 1237885 - Make browser_rules_add-rule_01.js faster by doing less; r=jdescottes

Review commit: https://reviewboard.mozilla.org/r/55884/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55884/
Attachment #8757456 - Flags: review?(jdescottes)
Comment on attachment 8757456 [details]
MozReview Request: Bug 1237885 - Make browser_rules_add-rule_01.js faster by doing less; r=jdescottes

https://reviewboard.mozilla.org/r/55884/#review52630

Thank you! Nothing to say about this changeset.

I have to raise one issue though: Bug 1275083 made browser_rules_add-rule_05.js totally redundant with browser_rules_add-rule_01.js.
It used to be the "context menu" version of browser_rules_add-rule_01, so it no longer has any purpose.
It has one less test case, maybe this is why it didn't show up yet as an intermittent.

It would be nice to take the opportunity to remove browser_rules_add-rule_05.js here.
Attachment #8757456 - Flags: review?(jdescottes) → review+
Thanks for the review. You're right, I'll take a look at all the add-rule_* tests to get a better understanding for what each of them do.
In the meantime: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2119baae6e71&group_state=expanded&selectedJob=21601358
I've retriggered a bunch of test jobs.

Comment 25

2 years ago
29 automation job failures were associated with this bug in the last 7 days.

Repository breakdown:
* mozilla-inbound: 21
* ash: 8

Platform breakdown:
* linux64: 23
* linux32: 6

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1237885&startday=2016-05-23&endday=2016-05-29&tree=all
I've checked all rule-view tests that add new rules and concluded that it's basically a real mess. Code duplication and overlapping tests everywhere :) I'm going to upload new patches to clean this up a bit.
Created attachment 8757870 [details]
MozReview Request: Bug 1237885 - Remove duplicated addNewRule code from ruleview tests; r=jdescottes

Review commit: https://reviewboard.mozilla.org/r/56240/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/56240/
Attachment #8757870 - Flags: review?(jdescottes)
Attachment #8757871 - Flags: review?(jdescottes)
Attachment #8757872 - Flags: review?(jdescottes)
Attachment #8757873 - Flags: review?(jdescottes)
Created attachment 8757871 [details]
MozReview Request: Bug 1237885 - Remove code duplication in new rule checks in ruleview tests; r=jdescottes

Review commit: https://reviewboard.mozilla.org/r/56242/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/56242/
Created attachment 8757872 [details]
MozReview Request: Bug 1237885 - Remove browser_rules_add-rule_05.js; r=jdescottes

This test is now useless if you consider
browser_rules_add-rule_01.js and
browser_rules_add-rule-and-property.js

Review commit: https://reviewboard.mozilla.org/r/56244/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/56244/
Created attachment 8757873 [details]
MozReview Request: Bug 1237885 - Renamed all add-rule tests to make intent clearer and avoid duplication; r=jdescottes

Review commit: https://reviewboard.mozilla.org/r/56246/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/56246/
Comment on attachment 8757456 [details]
MozReview Request: Bug 1237885 - Make browser_rules_add-rule_01.js faster by doing less; r=jdescottes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55884/diff/1-2/
Comment on attachment 8757872 [details]
MozReview Request: Bug 1237885 - Remove browser_rules_add-rule_05.js; r=jdescottes

https://reviewboard.mozilla.org/r/56244/#review52862

Thanks!
Attachment #8757872 - Flags: review?(jdescottes) → review+
Attachment #8757870 - Flags: review?(jdescottes) → review+
Comment on attachment 8757870 [details]
MozReview Request: Bug 1237885 - Remove duplicated addNewRule code from ruleview tests; r=jdescottes

https://reviewboard.mozilla.org/r/56240/#review52864
Comment on attachment 8757871 [details]
MozReview Request: Bug 1237885 - Remove code duplication in new rule checks in ruleview tests; r=jdescottes

https://reviewboard.mozilla.org/r/56242/#review52868

Thanks for the cleanup!
Attachment #8757871 - Flags: review?(jdescottes) → review+
Attachment #8757873 - Flags: review?(jdescottes) → review+
Comment on attachment 8757873 [details]
MozReview Request: Bug 1237885 - Renamed all add-rule tests to make intent clearer and avoid duplication; r=jdescottes

https://reviewboard.mozilla.org/r/56246/#review52870
Just one more try push to make sure I didn't break anything with the last patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=75c8a334af92&group_state=expanded
You need to log in before you can comment on or make changes to this bug.