Closed Bug 658450 Opened 12 years ago Closed 12 years ago

ONLY test_Scriptaculous.html | testHovering - 1 assertions, 1 failures, 0 errors without testInPlaceEditor, following plugin tests moving to dom/

Categories

(Core Graveyard :: Plug-ins, defect)

Product:
Core Graveyard
Component:
Plug-ins
Type:
defect
Priority:
Not set
Severity:
major

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla7

People

(Reporter: philor, Assigned: justin.lebar+bug)

References

Details

(Keywords: intermittent-failure, regression)

Attachments

(1 file)

Patch v1
4.24 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review 
Because we've had bug 510915 on /ignore for so long, we pretty much failed to notice that "just one of two tests failing is not two tests failing."

Starting with http://hg.mozilla.org/mozilla-central/rev/f5ac9367f1e1 from bug 644626 which moved the plugin tests from modules/plugins/ to dom/plugins/,

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1305855141.1305856715.19726.gz
Rev3 Fedora 12 mozilla-central debug test mochitests-2/5 on 2011/05/19 18:32:21 

6783 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/ajax/scriptaculous/test_Scriptaculous.html | testHovering - 1 assertions, 1 failures, 0 errors
Failure: should be transparent after mouse leaves element: expected "'transparent'", actual "'rgb(255, 255, 249)'"
WARNING: NS_ENSURE_SUCCESS(result, result) failed with result 0x80004005: file ../../../../editor/libeditor/base/nsEditor.cpp, line 3849
WARNING: NS_ENSURE_SUCCESS(res, res) failed with result 0x80004005: file ../../../../editor/libeditor/text/nsTextEditRules.cpp, line 461

To be sure I was blaming the right thing, I ran 11 Linux Md2s and 10 WinXP Md2s on the push before, and all 21 were green, and on this push I ran 7 Linux Md2s, of which 4 hit this, and 5 WinXP Md2s, none of which hit this, since apparently WinXP isn't quite as vulnerable - it tends closer to 1 in 5 failing, while Linux is closer to 4 in 5.
Well so, umm, can anyone explain how moving plugin tests (which by code inspection obviously doesn't do anything else) could cause this to fail, other than rearranging the order in which tests are run so that latent ordering issues may show up more often?

This sounds like a straight dup that could happen any time the tests are rearranged.
The first couple of things that occurred to me as possibilities were "some plugin test doesn't always clean up after itself" and "some plugin test continues running after it is supposed to be finished, sometimes throwing something up in the way of testHovering."

In their previous home in M4, nothing ran after them except parser/htmlparser/tests/, which might have not been bothered by a leftover that does bother Scriptaculous. (Or, since there are some apparently-baffling htmlparser test failures, maybe they were bothered by plugin leftovers, and will now be happy to have seen them move out of the neighborhood.)

I know it's trite to say that someone else should set up and use record-and-replay for something that you yourself are not willing to set it up and use it for, but, this has hit the last 5 Linux32 debug M2s in a row, it shouldn't be terribly hard to capture.
This test is bogus.  If it had been developed in-house, we'd back it out until someone could fix it.  I think we should do the same here.

While I agree in principal that it's good to have unit tests for this framework as part of our automated suite to ensure that we don't regress the framework, I have to imagine that we never would have added this test suite in the first place if we'd known that it was flaky.  I don't think the fact that it's already checked in should keep us from removing the flaky tests.

I'll attach a patch for this orange in a moment.
Attached patch Patch v1 — — Splinter Review 

        Attachment #535759 -
        Flags: review?(ehsan)

    
    

    
  
Comment on attachment 535759 [details] [diff] [review]
Patch v1

If we wanted to, we could just remove the code inside the wait()s.  I don't really think it matters either way; the code outside the wait()s looks like sanity checks for the most part.

    
    

    
  
Comment on attachment 535759 [details] [diff] [review]
Patch v1

This should also fix bug 510915.

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Comment on attachment 535759 [details] [diff] [review]
Patch v1

I concur!

        Attachment #535759 -
        Flags: review?(ehsan) → review+

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
http://hg.mozilla.org/mozilla-central/rev/059131725c45
Assignee: nobody → justin.lebar+bug
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla7