Check for any window left open after a browser-chrome test

RESOLVED FIXED in mozilla5

Status

defect
RESOLVED FIXED
9 years ago
Last year

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

Trunk
mozilla5
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(1 attachment)

Assignee

Description

9 years ago
I've noticed this a few times since yesterday.  See <http://tbpl.mozilla.org/?tree=MozillaTry&rev=d458b3eebe13> for an example (you may need to go back in time a few times to reach the revision.)  In the right bottom pane one sees:

    mochitest-chrome: 14062/0/112
    mochitest-browser-chrome: 13815/0/7
    mochitest-a11y: 15074/0/15
    mochitest-ipcplugins: 458/0/0

But 4 browser chrome tests have actually failed...
From the log:

INFO TEST-START | Shutdown
Browser Chrome Test Summary
	Passed: 13815
	Failed: 0
	Todo: 7

*** End BrowserChrome Test Results ***
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/browser_uninstalling.js | Tried to remove an add-on that wasn't registered with the mock provider
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/browser_uninstalling.js | Tried to remove an add-on that wasn't registered with the mock provider
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/browser_uninstalling.js | Tried to remove an add-on that wasn't registered with the mock provider
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/browser_uninstalling.js | Tried to remove an add-on that wasn't registered with the mock provider
gned.xpi to file C:\Users\cltbld\AppData\Local\Temp\tmp-jfu.xpi
Component: Tinderboxpushlog → BrowserTest
OS: Mac OS X → All
Product: Webtools → Testing
QA Contact: tinderboxpushlog → browsertest
Hardware: x86 → All
That's really odd. No idea how this could happen, unless it was caused by some JS bug with .map() or something. I can't seem to reproduce it.
Forgot to mention the STR:

1. hg up -r 965dc117adc0 (up through e7a1b719e090) (or revert http://hg.mozilla.org/mozilla-central/rev/1c42dfb39b1b)
2. have done that on Windows
Assignee

Comment 4

9 years ago
(In reply to comment #3)
> Forgot to mention the STR:
> 
> 1. hg up -r 965dc117adc0 (up through e7a1b719e090) (or revert
> http://hg.mozilla.org/mozilla-central/rev/1c42dfb39b1b)

Do you mean this is a regression from 1c42dfb39b1b?

> 2. have done that on Windows

...and?
Assignee

Comment 5

9 years ago
(In reply to comment #2)
> That's really odd. No idea how this could happen, unless it was caused by some
> JS bug with .map() or something. I can't seem to reproduce it.

Actually, unless I'm on crack, those four failures seem to have happened *after* this call was made: <http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/browser-test.js#126>, so that very well explains the 0... ;-)
Assignee

Comment 6

9 years ago
Mossop might have some ideas here...
The problem is quite simple, the failures are happening after the browser-chrome test run has completed and the summary is printed out. In the case here one of the tests left an add-ons manager window open that was still holding some event listeners keeping the old test scope alive. When the window got closed by the application shutting down the test scope triggered a couple of failures.
Unless you make run_tests.py create the summary you really don't have a way to avoid this. I don't think it is worth fixing tbh, it was still turning the boxes orange ok, after that the summary count is just a nicety
Assignee

Comment 9

9 years ago
(In reply to comment #7)
> The problem is quite simple, the failures are happening after the
> browser-chrome test run has completed and the summary is printed out. In the
> case here one of the tests left an add-ons manager window open that was still
> holding some event listeners keeping the old test scope alive. When the window
> got closed by the application shutting down the test scope triggered a couple
> of failures.

Is it possible to use registerCleanupFuncton in add-on manager tests so that we catch the test which failed to close the window?  A window left open might lead to other failures as well, I guess.
(In reply to comment #9)
> (In reply to comment #7)
> > The problem is quite simple, the failures are happening after the
> > browser-chrome test run has completed and the summary is printed out. In the
> > case here one of the tests left an add-ons manager window open that was still
> > holding some event listeners keeping the old test scope alive. When the window
> > got closed by the application shutting down the test scope triggered a couple
> > of failures.
> 
> Is it possible to use registerCleanupFuncton in add-on manager tests so that we
> catch the test which failed to close the window?  A window left open might lead
> to other failures as well, I guess.

Yes, I did so and fixed the test that was doing it in http://hg.mozilla.org/mozilla-central/rev/1c42dfb39b1b.

I was a little surprised that the browser chrome harness itself wasn't failing on extra windows though, I thought it did that anyway
Assignee

Comment 11

9 years ago
So does this mean that we shouldn't see this problem any more?
I don't believe so. The changeset references in comment 0 contains the change that caused the failure and not the change that fixed it. If this is still happening at all then there is some other reason for it that I'm not aware of.
waitForWindowState should be catching windows left open by broken/timedout tests:

http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/browser-test.js#92
(In reply to comment #13)
> waitForWindowState should be catching windows left open by broken/timedout
> tests:
> 
> http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/browser-test.js#92

That checks for browser windows, not for any other kind though.
Assignee

Comment 15

9 years ago
(In reply to comment #14)
> (In reply to comment #13)
> > waitForWindowState should be catching windows left open by broken/timedout
> > tests:
> > 
> > http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/browser-test.js#92
> 
> That checks for browser windows, not for any other kind though.

Can't we pass null to getEnumerator there to look for every window type (excluding the browser-chrome tests window itself)?
Assignee

Comment 16

9 years ago
Posted patch Patch (v1)Splinter Review
(In reply to comment #15)
> (In reply to comment #14)
> > (In reply to comment #13)
> > > waitForWindowState should be catching windows left open by broken/timedout
> > > tests:
> > > 
> > > http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/browser-test.js#92
> > 
> > That checks for browser windows, not for any other kind though.
> 
> Can't we pass null to getEnumerator there to look for every window type
> (excluding the browser-chrome tests window itself)?

Let me put my patch where my mouth is!
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #486960 - Flags: review?(gavin.sharp)
Assignee

Updated

9 years ago
Summary: Browser chrome failure count incorrectly reported as 0 → Check for any window left open after a browser-chrome test
Assignee

Updated

9 years ago
Depends on: 609521
Assignee

Comment 17

9 years ago
Gavin: ping?
Assignee

Comment 18

8 years ago
Gavin: ping times two? ;)
Comment on attachment 486960 [details] [diff] [review]
Patch (v1)

>diff --git a/testing/mochitest/browser-test.js b/testing/mochitest/browser-test.js

>+        let type = win.document.documentElement.getAttribute("windowtype");
>+        switch (type) {
>+        case "navigator:browser":
>+          type = "browser window";
>+          break;
>+        case null:
>+          type = "unknown window";

type = "unknown window with type: " + type; // ?
Attachment #486960 - Flags: review?(gavin.sharp) → review+
Assignee

Comment 20

8 years ago
http://hg.mozilla.org/mozilla-central/rev/678e2a2d2195
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.2
Component: BrowserTest → Mochitest
You need to log in before you can comment on or make changes to this bug.