887315 - Mutt test window_focus.js fails with: "Windows have maintained their order - '[3,31,41]' should equal '[3,41,31]'

Status: RESOLVED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

[Jonathan French (:jfrench)]

Attachment: window_focus failure xp32 sp3

I'm observing that window_focus.js now fails in a mutt test run, in the form:

mutt -m mutt/mutt/tests/js/controller/tests.ini -b (binary)

At least on my local WindowsXP32 SP3 client, with current Nightly 25.0a1 20130625031238 as the binary. I've provided an attachment of a run using mutt -v for full output, with the other preceding(passing) tests in log stripped for clarity.

Assert error also included below for the thread
"message": "Windows have maintained their order - '[3,31,41]' should equal '[3,41,31]'"

Hopefully others can reproduce it also.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I have seen the same on OS X. Not sure if this is expected or not. But looks like in both cases we get a different order of the windows. The test has been introduced with the patch on bug 803492.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Over on bug 803492 Neil suggested a patch so we do not set the visibility on the window if we are in testing mode. It's attachment 765942 [details] [diff] [review].

I think there is a bug in there given that it might have to be isTestMode. Neil, can you please clarify? I think it would be good to create a tryserver build so that we can test this all.

Comment 3

[Andrei Eftimie]

I haven't got around to make a custom FF build to test it with Neil's patch.
Will try it today to see if it makes any difference.

Comment 4

[Andrei Eftimie]

Just tested Neils patch from bug 803492
It works! Windows are no longer reordered by calling window.focus().

Now how do we get this into FF?
Should I log a new bug for that and attach Neil's patch?

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

That's great. I would indeed suggest that we file a new bug in Core:DOM (similar to bug 704583). I would let Neil upload the patch and to ask the right person for review. Please mark the dependencies.

Comment 6

[Andrei Eftimie]

I've opened bug 887718 to get this fixed in Firefox.
Thanks Neil

Comment 7

[Jonathan French (:jfrench)]

The blocking fixed bug 887718 was returned against Core on 2013-07-05 here
https://hg.mozilla.org/mozilla-central/rev/d6ca24ba3673

but I continue to see the "windows have maintained their order" failure 2 build days after the return, running mutt -m ..controller/tests.ini in

Windows XP SP3
Mozmill2.0rc4 latest
latest Nightly 25.0a1 20130707031138 (<- 2013-07-07)

Perhaps Andrei can try it out and see if he sees the same thing. Maybe I misunderstand the nature of sTestMode in the return, or the timing of when landed Core code ends up in the build.

Comment 8

[Jonathan French (:jfrench)]

(In reply to Jonathan French (:jfrench) from comment #7)
> Maybe I misunderstand the timing of when landed Core code ends up in the build.

Actually specific to this, I was looking for a number higher than the related changelog 137546 in the Troubleshooting Information in the FF browser. I didn't find any on the main panel, but on digging in about:buildconfig it appears 2013-07-07 is built off 137496; so the verification was just a bit early (50 changes). Perhaps tomorrow's build.

I didn't realize there was a multi day delay between a Core return and its presence in the build. Ditto for the blocking bug being marked fixed prior.

Comment 9

[Andrei Eftimie]

Jonathan, don't worry about it so much.

Once code gets landed or merged in mozilla-central you usually see it in the next day's build.
I planned to wait until tomorrows build to test this has been properly fixed.

But you can see it in tinderbox-builds which get triggered for each commit.
For this case I took the latest tinderbox build available at this time: https://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-macosx64/1373246384/

And the mutt test now passes.

Comment 10

[Andrei Eftimie]

Attachment: unskip test under linux

I've unskipped this test under linux. 
The quoted issue - bug 462222 - does not appear to have anything to do with it failing. It failed under linux for the same reason it failed under OSX and Windows. And that has been fixed now.

Comment 11

[Andrei Eftimie]

I failed to mention, but if it's not obvious from the previous comment: the mutt test correctly passes for me under linux.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 771940 [details] [diff] [review]
unskip test under linux

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

Problem here is that when we enable the test it will still fail with other versions of Firefox. mozinfo should allow us to retrieve the version from the application and disable conditionally. I don't think there is a bug for it yet. Would you mind to file it?

As of now please update the test, so it only gets run with a gecko version >= 25.0. Otherwise we should skip it inside the test.

Comment 13

[Andrei Eftimie]

Attachment: unskip for linux and skip for FF older then 25

Now skipped for versions older than 25.

> mozinfo should allow us to retrieve the version from the application and disable 
> conditionally. I don't think there is a bug for it yet. Would you mind to file
> it?

I'm assuming you mean the manifestparser here: https://github.com/andreieftimie/mozbase/blob/master/manifestdestiny/manifestparser/manifestparser.py

So we can use it in tests.ini like:
> [sampleTest.js]
> skip-if = app-version <= 25 = "My message for skipping"

Please let me know if you thought of something else.

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Andrei Eftimie from comment #13)
> I'm assuming you mean the manifestparser here:
> https://github.com/andreieftimie/mozbase/blob/master/manifestdestiny/
> manifestparser/manifestparser.py
> 
> So we can use it in tests.ini like:
> > [sampleTest.js]
> > skip-if = app-version <= 25 = "My message for skipping"
> 
> Please let me know if you thought of something else.

All this comes from mozinfo. manifestparser is just the consumer.

Comment 15

[Jonathan French (:jfrench)]

(In reply to Andrei Eftimie from comment #9)
> 
> And the mutt test now passes.

It's also passing consistently for me too, as expected with the new Nightly.

Windows XP SP3
Mozmill2.0rc4 latest
latest Nightly 25.0a1 20130708031114 CL 137589

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 772087 [details] [diff] [review]
unskip for linux and skip for FF older then 25

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

::: mutt/mutt/tests/js/controller/window_focus.js
@@ +6,4 @@
>  
>  const BASE_URL = collector.addHttpResource("../_files/");
>  const TEST_DATA = BASE_URL + "complex.html";
> +const APP_VERSION = parseInt(Services.appinfo.version.split('.')[0]);

No need to split out the .0 part of the version. Instead make use of the version comparator which can correctly compare different versions. Also I don't see a reason why we need a constant here.

@@ +54,5 @@
>    aModule.windowController2.window.close();
>  }
> +
> +if (APP_VERSION < 25) {
> +  setupModule.__force_skip__ = "Only works in Firefox 25 and newer";

The bug reference is missing.

Comment 17

[Andrei Eftimie]

Attachment: unskip for linux and skip for FF older then 25 v2

Used version comparator directly and added bug reference to bug 887718 (where the change has been introduced)

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 773176 [details] [diff] [review]
unskip for linux and skip for FF older then 25 v2

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

::: mutt/mutt/tests/js/controller/tests.ini
@@ -10,4 @@
>  [screenshot.js]
>  [synthesize_events.js]
>  [window_focus.js]
> -skip-if = os == 'linux' = Bug 462222 - getZOrderDOMWindowEnumerator is broken on Linux

Why do we still have to skip it on Linux? As you said in comment 13 you wanted to unskip the patch for Linux.

::: mutt/mutt/tests/js/controller/window_focus.js
@@ +52,5 @@
>    aModule.windowController1.window.close();
>    aModule.windowController2.window.close();
>  }
> +
> +if (Services.vc.compare(Services.appinfo.version, 24) <= 0) {

Please use 25 and < here. Otherwise we will fail for ESR builds which will become 24.x.y.

@@ +54,5 @@
>  }
> +
> +if (Services.vc.compare(Services.appinfo.version, 24) <= 0) {
> +  setupModule.__force_skip__ = "Bug 887718 - Only works in Firefox 25 and newer";
> +  teardownModule.__force_skip__ = "Bug 887718 - Only works in Firefox 25 and newer";

Well, I still want to see a better message here which indeed includes that the window order is not correctly returned before Firefox 25.

Comment 19

[Andrei Eftimie]

Attachment: unskip for linux and skip for FF older then 25 v3

(In reply to Henrik Skupin (:whimboo) from comment #18)
> Comment on attachment 773176 [details] [diff] [review]
> unskip for linux and skip for FF older then 25 v2
> 
> Review of attachment 773176 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mutt/mutt/tests/js/controller/tests.ini
> @@ -10,4 @@
> >  [screenshot.js]
> >  [synthesize_events.js]
> >  [window_focus.js]
> > -skip-if = os == 'linux' = Bug 462222 - getZOrderDOMWindowEnumerator is broken on Linux
> 
> Why do we still have to skip it on Linux? As you said in comment 13 you
> wanted to unskip the patch for Linux.

Uhm, this is unskipped for linux...

> ::: mutt/mutt/tests/js/controller/window_focus.js
> @@ +52,5 @@
> >    aModule.windowController1.window.close();
> >    aModule.windowController2.window.close();
> >  }
> > +
> > +if (Services.vc.compare(Services.appinfo.version, 24) <= 0) {
> 
> Please use 25 and < here. Otherwise we will fail for ESR builds which will
> become 24.x.y.

This does not work with "25"
> Services.appinfo.version // "25.0a1"
> Services.vc.compare("25.0a1", "25") // -1

So the version comparator sees the latest nightly as alpha, and tell us that it is older than 25.
As a working solution I've used "25.0a":
> Services.vc.compare("25.0a1", "25.0a") // 1

> @@ +54,5 @@
> >  }
> > +
> > +if (Services.vc.compare(Services.appinfo.version, 24) <= 0) {
> > +  setupModule.__force_skip__ = "Bug 887718 - Only works in Firefox 25 and newer";
> > +  teardownModule.__force_skip__ = "Bug 887718 - Only works in Firefox 25 and newer";
> 
> Well, I still want to see a better message here which indeed includes that
> the window order is not correctly returned before Firefox 25.

Included more information on the issue in the description

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 773852 [details] [diff] [review]
unskip for linux and skip for FF older then 25 v3

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

::: mutt/mutt/tests/js/controller/window_focus.js
@@ +52,5 @@
>    aModule.windowController1.window.close();
>    aModule.windowController2.window.close();
>  }
> +
> +if (Services.vc.compare(Services.appinfo.version, "25.0a") < 0) {

Hm, I would still use 25.0a1 here. Those are the first builds we get for 25.0 and it should work perfect, right? With that fixed you get r+.

Comment 21

[Andrei Eftimie]

Attachment: unskip for linux and skip for FF older then 25 v4

Works fine with "25.0a1" :)

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Landed on master:
https://github.com/mozilla/mozmill/commit/f2f7dd4c67ab95a1b3e10d2867a79a561447be7c