browser-test.js should ensure windows state between tests

RESOLVED FIXED in mozilla1.9.3a1

Status

Testing
Mochitest
RESOLVED FIXED
9 years ago
5 months ago

People

(Reporter: mak, Assigned: mak)

Tracking

Trunk
mozilla1.9.3a1
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

9 years ago
Created attachment 412195 [details] [diff] [review]
patch v1.0

Before executing next test it should ensure all closing windows are closed and there are no leftover windows from previous tests.

See bug 528452 and bug 527074 for background info that brought to this request.

this is a first attempt, looking for feedback.
(Assignee)

Comment 1

9 years ago
Created attachment 412197 [details] [diff] [review]
patch v1.1

clearly without stupid dumps :)
Attachment #412195 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Assignee: nobody → mak77
Status: NEW → ASSIGNED
(Assignee)

Comment 2

9 years ago
Created attachment 412198 [details] [diff] [review]
patch v1.2

this works, stupid error.
Attachment #412197 - Attachment is obsolete: true
(Assignee)

Comment 3

9 years ago
Comment on attachment 412198 [details] [diff] [review]
patch v1.2

looking for feedback.
Attachment #412198 - Flags: review?(dao)
(Assignee)

Updated

9 years ago
Attachment #412198 - Flags: review?(gavin.sharp)
Comment on attachment 412198 [details] [diff] [review]
patch v1.2

>+        setTimeout(function() {
>+          self.waitForWindowsState(aCallback);
>+        }, 100);

Why 100 rather than 0?

>+        return;
>+      }
>+    }
>+    window.focus();
>+    this.SimpleTest.waitForFocus(function() {
>+      aCallback.apply(self);
>+    });

I think waitForFocus uses setTimeout(..., 0) even if the window is already focused, so this could slow down the whole browser chrome testsuite a bit.

The general approach looks ok to may, Gavin should do the full review.
Attachment #412198 - Flags: review?(dao)
> The general approach looks ok to may

Erm... it looks ok to me.
(Assignee)

Comment 6

9 years ago
i just wanted to ensure focus, i think waitForFocus if the window is already focused won't wait. that said, losing a minute against greener tboxes is still a win probably.
(Assignee)

Comment 7

9 years ago
oh i misread the comment, yes if it does setTimeout(, 0) we could lose some ms, my conclusion in comment 6 can still be valid though.
(Assignee)

Comment 8

9 years ago
(In reply to comment #4)
> (From update of attachment 412198 [details] [diff] [review])
> >+        setTimeout(function() {
> >+          self.waitForWindowsState(aCallback);
> >+        }, 100);
> 
> Why 100 rather than 0?

retrying immediately could just bring to loops that will overload the cpu, that being virtualized will steal cycles to other VMs, likely causing more headaches.
It could be reduced to 50ms eventually.
0 doesn't actually mean immediately.
(Assignee)

Comment 10

9 years ago
i know, but enqueued is too early still, imo.
I think it's pretty much what we want. In any case, there's still a minimum delay for nested timeouts.
(Assignee)

Comment 12

9 years ago
i pushed the patch to the tryserver along with other patches with a 50ms delay, and everything went fine afaict.
see also bug 521282
Comment on attachment 412198 [details] [diff] [review]
patch v1.2

>+      if (win.closed) {
>+        setTimeout(function() {
>+          self.waitForWindowsState(aCallback);
>+        }, 100);
>+        return;
>+      }

On second thought, I don't think we should do this. It would hide bugs like bug 528776.
Comment on attachment 412198 [details] [diff] [review]
patch v1.2

Yeah, I agree. Not doing it might expose some existing issues, but we should fix them.

Looks like this needs to be updated to take into account bug 521282 (no longer need the waitForFocus call).

I worry a bit that having to getService/getEnumerator for each test will slow down these test runs, have you tested that impact? Could keep a reference to windowMediator, but that's likely insignificant compared to getEnumerator... or maybe it's all insignificant.

Why was the change to observe() needed? It doesn't hurt to be more resilient there, but there seems to be very little that can happen between the listener being registered and currentTest being set...
Attachment #412198 - Flags: review?(gavin.sharp)
(Assignee)

Comment 16

9 years ago
(In reply to comment #15)
> (From update of attachment 412198 [details] [diff] [review])
> Yeah, I agree. Not doing it might expose some existing issues, but we should
> fix them.
> 
> Looks like this needs to be updated to take into account bug 521282 (no longer
> need the waitForFocus call).

Ok, so we can just check if there are windows that are not marked as closed, and notify that as an error.

> I worry a bit that having to getService/getEnumerator for each test will slow
> down these test runs, have you tested that impact? Could keep a reference to
> windowMediator, but that's likely insignificant compared to getEnumerator... or
> maybe it's all insignificant.

I've not noticed a large slowdown, i guess i could measure the difference before and after.

> Why was the change to observe() needed? It doesn't hurt to be more resilient
> there, but there seems to be very little that can happen between the listener
> being registered and currentTest being set...

Well while working on this i got an error about this.currentTest being undefined there, so i thought was saner to handle such a case.
(Assignee)

Comment 17

9 years ago
Created attachment 414070 [details] [diff] [review]
patch v1.3

if you're fine with this, i prefer having a single method that will take care of checking status between tests, that's why i've moved the focus changes inside my waitForWindowsState method. Also because ensuring focus has nothing to do with the test itself, but is something that checks before executing the test, semantically i find this more correct and having code in the same place should make it even more maintanable (supposing in future we could use waitForFocus reliably).

I cached services used in different places or multiple times in the Tester object.

With this i measured time needed to run Sessionstore browser-chrome tests, and i did not see any perf loss, time is practically the same (85s on my platform).
Attachment #412198 - Attachment is obsolete: true
Attachment #414070 - Flags: review?(gavin.sharp)
The browser windows check should probably happen after each test, not before. This matters for the last test, which could otherwise be broken but would only fail when another tests gets added. And then it looks like this.currentTest.addResult should be used.
(Assignee)

Comment 19

9 years ago
that's an edge case, but would be easily fixable like this

let callback = this.tests.length > 0 ? this.execTest : this.finish;
this.waitForWindowsState(callback);

this way we also ensure the harness starts with right number of windows.

I'm not sure if the error should be reported as a current test error, this code is not part of the current test, even if it's checking what current test left behind...
(Assignee)

Comment 20

9 years ago
Created attachment 414085 [details] [diff] [review]
patch v1.4

fixes the above comments. or i could just check after test run, skipping initial check, as dao suggested. we just miss the initial check, could be just paranoid.
Attachment #414070 - Attachment is obsolete: true
Attachment #414070 - Flags: review?(gavin.sharp)
(Assignee)

Updated

9 years ago
Attachment #414085 - Flags: review?(gavin.sharp)
(Assignee)

Comment 21

9 years ago
pushed to the tryserver and got green
Created attachment 421753 [details] [diff] [review]
tweaked patch

Largely the same, just with some ordering tweaks to make sure we check once at the start, and also after every test (making sure to associate the failure with the correct test).
Attachment #414085 - Attachment is obsolete: true
Attachment #421753 - Flags: review?(mak77)
Attachment #414085 - Flags: review?(gavin.sharp)
tryservered that patch and got green.
(Assignee)

Comment 24

9 years ago
Comment on attachment 421753 [details] [diff] [review]
tweaked patch

yeah, that was the original intent, looks like i got confused by the fancy structure of the tester and hooked too early. This structure looks clearer too.
Attachment #421753 - Flags: review?(mak77) → review+
(Assignee)

Comment 25

9 years ago
i'll check my other b-c changes in case they need an unbitrot.
(Assignee)

Updated

9 years ago
Flags: in-testsuite?
(Assignee)

Updated

9 years ago
Blocks: 485269, 529860
(Assignee)

Comment 26

9 years ago
http://hg.mozilla.org/mozilla-central/rev/9b9fa1cc982e
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
(Assignee)

Updated

3 years ago
Flags: in-testsuite? → in-testsuite+
Component: BrowserTest → Mochitest
Product: Testing → Testing
You need to log in before you can comment on or make changes to this bug.