Closed Bug 825410 Opened 12 years ago Closed 9 years ago

Intermittent browser_inspector_pseudoclass_lock.js | Test timed out

Categories

(DevTools :: Inspector, defect)

Product:
DevTools
Component:
Inspector
Platform:
All
Windows 7
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox29 wontfix, firefox30 fixed, firefox31 fixed, firefox-esr24 wontfix)

Status:
RESOLVED WORKSFORME
Milestone:
Firefox 31
Tracking Flags:
Tracking Status
firefox29 --- wontfix
firefox30 --- fixed
firefox31 --- fixed
firefox-esr24 --- wontfix

People

(Reporter: graememcc, Assigned: pbro)

Details

(Keywords: intermittent-failure)

Attachments

(1 file, 2 obsolete files)

bug825410-intermittent-browser_inspector_pseudoclass_lock.js v3.patch
20.16 KB, patch
pbro
: review+
Details | Diff | Splinter Review 
https://tbpl.mozilla.org/php/getParsedLog.php?id=18334352&tree=Mozilla-Inbound

Rev3 WINNT 6.1 mozilla-inbound opt test mochitest-browser-chrome on 2012-12-29 02:15:25 PST for push ef2a48aded5b
slave: talos-r3-w7-061

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/inspector/test/browser_inspector_pseudoclass_lock.js | Test timed out
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/inspector/test/browser_inspector_pseudoclass_lock.js | Found a tab after previous test timed out: data:text/html,pseudo-class%20lock%20tests
The error I'm seeing in the log is the same one that was causing most of the timeouts in the styleinspector tests a couple of weeks ago and that went away with bug 988313 fixed:
`Full message: TypeError: inspector.sidebar.getWindowForTab(...).ruleview is undefined`
I have created bug 988314 for doing the same with the inspector tests, but right now, perhaps we should just work around that bug quickly before spending 3 weeks cleaning tests up.
Assignee: nobody → pbrosset
Ongoing try build: https://tbpl.mozilla.org/?tree=Try&rev=08a69fed906e

In this patch, I rewrote the openInspector function in head.js and added an openRuleView function.
The code for these comes from the styleinspector tests' head.js which was cleaned up last week, so this intermittent should go away.
I've also rewrote the failing test to remove the very deep callback nesting that was there.
And also had to touch a few other tests that were impacted by my openInspector changes.
Let's see on try.
https://tbpl.mozilla.org/?tree=Try&rev=08a69fed906e looks good so far. The OSX 10.8 and linux 64 debug builds take an awful lot of time to start, but I'm confident they'll turn out green too.
Attachment #8406711 - Flags: review?(bgrinstead)
Comment on attachment 8406711 [details] [diff] [review]
bug825410-intermittent-browser_inspector_pseudoclass_lock.js v1.patch

Not the right patch.
Attachment #8406711 - Flags: review?(bgrinstead)

        Attachment #8406711 -
        Attachment is obsolete: true

        Attachment #8406952 -
        Flags: review?(bgrinstead)

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Comment on attachment 8406952 [details] [diff] [review]
bug825410-intermittent-browser_inspector_pseudoclass_lock.js v2.patch

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

OK, so the main changes here are having the openInspector function wait for inspector-updated rather than having each test wait.  I'm assuming these are the only tests in inspector/ that were doing this.

I've checked the changes in everything expect for browser_inspector_pseudoclass_lock.js, and it all looks good so far.  I'll update once I finish looking at the changes to that test.

I pushed the patch to try: https://tbpl.mozilla.org/?tree=Try&rev=a226972c2328

::: browser/devtools/inspector/test/head.js
@@ +188,5 @@
> +function hasSideBarTab(inspector, id) {
> +  return !!inspector.sidebar.getWindowForTab(id);
> +}
> +
> +// function openInspector(callback)

Commented out code that can be removed

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Comment on attachment 8406952 [details] [diff] [review]
bug825410-intermittent-browser_inspector_pseudoclass_lock.js v2.patch

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

This code for the pseudoclass lock is much better than the current test.  I don't really have much to add to this besides a couple of inline comments.

::: browser/devtools/inspector/test/browser_inspector_pseudoclass_lock.js
@@ +29,5 @@
> +  yield finishUp(toolbox);
> +  finish();
> +});
> +
> +function createDocument() {

Is there a reason createDocument is building the DOM in this way vs specifying the same through markup?  Seems like doing something like this would be easier to read and less lines of code:

content.location = 'data:text/html,' +
'<head><style>div { color: red; } div:hover { color: blue; }</style></head>' +
'<body>pseudo-class lock tests' +
  '<div id="parent-div">parent div' +
    '<div id="div-1">test div</div>' +
    '<div id="div-2">test div2</div>' +
  '</div>' +
'</body>'

        Attachment #8406952 -
        Flags: review?(bgrinstead) → review+

    
    

    
  
Thanks Brian for the review. You're right, markup would be far easier to read.
For info, although I originally attached the wrong patch, the try build was correct though: https://tbpl.mozilla.org/?tree=Try&rev=08a69fed906e

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  


  
Differences from v2:
- removed commented out code in head.js as suggested by Brian
- removed the createDocument function in the test and replaced by inline markup, as suggested by Brian too.
Try builds are green. Going to push this in.

        Attachment #8406952 -
        Attachment is obsolete: true

        Attachment #8407369 -
        Flags: review+

    
    

    
  
Fixed in fx-team: https://hg.mozilla.org/integration/fx-team/rev/d80acdb6ce2c
Whiteboard: [fixed-in-fx-team]

    
    

    
  
If we want this uplifted to Aurora, it depends on bug 943510, otherwise it won't apply cleanly.
However, my patch changes the very line that was changed in bug 943510 anyway, so it might be better to provide a separate patch for Aurora uplift, unless we want to uplift bug 943510 too.
Ryan, thoughts on this?
Flags: needinfo?(ryanvm)

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #280)
> Ryan, thoughts on this?

Per our IRC chat, the plan is to uplift bug 943510 as part of a larger patch queue.
Flags: needinfo?(ryanvm)

    
    

    
      
  

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/d80acdb6ce2c
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 31

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
  
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Inactive; closing (see bug 1180138).
Status: REOPENED → RESOLVED
Closed: 10 years ago → 9 years ago
Resolution: --- → WORKSFORME

    
  
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: