Closed Bug 887315 Opened 7 years ago Closed 7 years ago

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

Categories

(Testing Graveyard :: Mozmill, defect)

defect
Not set
major

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jfrench, Assigned: andrei)

References

()

Details

(Whiteboard: [mozmill-2.0][ateamtrack: p=mozmill q=2013q3 m=1])

Attachments

(2 files, 4 obsolete files)

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.
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.
Depends on: 803492
OS: Windows XP → All
Hardware: x86 → All
Summary: Mutt test window_focus.js appears to fail in recent runs → Mutt test window_focus.js fails with: "Windows have maintained their order - '[3,31,41]' should equal '[3,41,31]'
Whiteboard: [mozmill-2.0] [lang=js]
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.
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.
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?
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.
Depends on: 887718
I've opened bug 887718 to get this fixed in Firefox.
Thanks Neil
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.
(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.
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.
Assignee: nobody → andrei.eftimie
Status: NEW → ASSIGNED
Attached patch unskip test under linux (obsolete) — Splinter Review
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.
Attachment #771940 - Flags: review?(hskupin)
I failed to mention, but if it's not obvious from the previous comment: the mutt test correctly passes for me under linux.
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.
Attachment #771940 - Flags: review?(hskupin) → review-
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.
Attachment #771940 - Attachment is obsolete: true
Attachment #772087 - Flags: review?(hskupin)
(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.
(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 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.
Attachment #772087 - Flags: review?(hskupin) → review-
Used version comparator directly and added bug reference to bug 887718 (where the change has been introduced)
Attachment #772087 - Attachment is obsolete: true
Attachment #773176 - Flags: review?(hskupin)
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.
Attachment #773176 - Flags: review?(hskupin) → review-
(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
Attachment #773176 - Attachment is obsolete: true
Attachment #773852 - Flags: review?(hskupin)
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+.
Attachment #773852 - Flags: review?(hskupin) → review+
Works fine with "25.0a1" :)
Attachment #773852 - Attachment is obsolete: true
Attachment #773905 - Flags: review?(hskupin)
Attachment #773905 - Flags: review?(hskupin) → review+
Landed on master:
https://github.com/mozilla/mozmill/commit/f2f7dd4c67ab95a1b3e10d2867a79a561447be7c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [mozmill-2.0][ateamtrack: p=mozmill q=2013q2 m=4]
Whiteboard: [mozmill-2.0][ateamtrack: p=mozmill q=2013q2 m=4] → [mozmill-2.0][ateamtrack: p=mozmill q=2013q3 m=1]
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.