Closed Bug 528469 Opened 15 years ago Closed 14 years ago

browser-test.js should ensure windows state between tests

Categories

(Testing :: Mochitest, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.9.3a1

People

(Reporter: mak, Assigned: mak)

References

Details

Attachments

(1 file, 5 obsolete files)

Attached patch patch v1.0 (obsolete) — Splinter Review
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.
Attached patch patch v1.1 (obsolete) — Splinter Review
clearly without stupid dumps :)
Attachment #412195 - Attachment is obsolete: true
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Attached patch patch v1.2 (obsolete) — Splinter Review
this works, stupid error.
Attachment #412197 - Attachment is obsolete: true
Comment on attachment 412198 [details] [diff] [review]
patch v1.2

looking for feedback.
Attachment #412198 - Flags: review?(dao)
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.
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.
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.
(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.
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.
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)
(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.
Attached patch patch v1.3 (obsolete) — Splinter Review
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.
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...
Attached patch patch v1.4 (obsolete) — Splinter Review
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)
Attachment #414085 - Flags: review?(gavin.sharp)
pushed to the tryserver and got green
Attached patch tweaked patchSplinter Review
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.
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+
i'll check my other b-c changes in case they need an unbitrot.
Flags: in-testsuite?
Blocks: 485269, 529860
http://hg.mozilla.org/mozilla-central/rev/9b9fa1cc982e
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
Flags: in-testsuite? → in-testsuite+
Component: BrowserTest → Mochitest
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: