Closed Bug 1237885 Opened 8 years ago Closed 8 years ago

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

Categories

(DevTools :: Inspector: Rules, defect, P2)

Product:
DevTools
Component:
Inspector: Rules
Type:
defect

Tracking

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

Status:
RESOLVED FIXED
Milestone:
Firefox 47
Tracking Flags:
Tracking Status
firefox45 --- unaffected
firefox46 --- fixed
firefox47 --- fixed
firefox49 --- fixed
firefox-esr45 --- unaffected

People

(Reporter: philor, Assigned: jdescottes)

References

Details

(Keywords: intermittent-failure)

Attachments

(6 files, 1 obsolete file)

bug1237885.v2.patch (rebased and applied review comments)
10.61 KB, patch
jdescottes
: review+
Details | Diff | Splinter Review
MozReview Request: Bug 1237885 - Make browser_rules_add-rule_01.js faster by doing less; r=jdescottes
58 bytes, text/x-review-board-request
jdescottes
: review+
Details
MozReview Request: Bug 1237885 - Remove duplicated addNewRule code from ruleview tests; r=jdescottes
58 bytes, text/x-review-board-request
jdescottes
: review+
Details
MozReview Request: Bug 1237885 - Remove code duplication in new rule checks in ruleview tests; r=jdescottes
58 bytes, text/x-review-board-request
jdescottes
: review+
Details
MozReview Request: Bug 1237885 - Remove browser_rules_add-rule_05.js; r=jdescottes
58 bytes, text/x-review-board-request
jdescottes
: review+
Details
MozReview Request: Bug 1237885 - Renamed all add-rule tests to make intent clearer and avoid duplication; r=jdescottes
58 bytes, text/x-review-board-request
jdescottes
: review+
Details 
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
Attached patch bug1237885.v1.patch (obsolete) — — Splinter Review 
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 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+
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+
https://hg.mozilla.org/mozilla-central/rev/05f74b15d9cf
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
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 → ---
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.
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.
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.
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/
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
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: