Closed
Bug 528469
Opened 15 years ago
Closed 14 years ago
browser-test.js should ensure windows state between tests
Categories
(Testing :: Mochitest, defect)
Testing
Mochitest
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla1.9.3a1
People
(Reporter: mak, Assigned: mak)
References
Details
Attachments
(1 file, 5 obsolete files)
9.06 KB,
patch
|
mak
:
review+
|
Details | Diff | 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.
Assignee | ||
Comment 1•15 years ago
|
||
clearly without stupid dumps :)
Attachment #412195 -
Attachment is obsolete: true
Assignee | ||
Updated•15 years ago
|
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•15 years ago
|
||
this works, stupid error.
Attachment #412197 -
Attachment is obsolete: true
Assignee | ||
Comment 3•15 years ago
|
||
Comment on attachment 412198 [details] [diff] [review] patch v1.2 looking for feedback.
Attachment #412198 -
Flags: review?(dao)
Assignee | ||
Updated•15 years ago
|
Attachment #412198 -
Flags: review?(gavin.sharp)
Comment 4•15 years ago
|
||
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)
Comment 5•15 years ago
|
||
> The general approach looks ok to may
Erm... it looks ok to me.
Assignee | ||
Comment 6•15 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•15 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•15 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.
Comment 9•15 years ago
|
||
0 doesn't actually mean immediately.
Assignee | ||
Comment 10•15 years ago
|
||
i know, but enqueued is too early still, imo.
Comment 11•15 years ago
|
||
I think it's pretty much what we want. In any case, there's still a minimum delay for nested timeouts.
Assignee | ||
Comment 12•15 years ago
|
||
i pushed the patch to the tryserver along with other patches with a 50ms delay, and everything went fine afaict.
Comment 13•15 years ago
|
||
see also bug 521282
Comment 14•15 years ago
|
||
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 15•15 years ago
|
||
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•15 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•15 years ago
|
||
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)
Comment 18•15 years ago
|
||
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•15 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•15 years ago
|
||
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•15 years ago
|
Attachment #414085 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 21•15 years ago
|
||
pushed to the tryserver and got green
Comment 22•14 years ago
|
||
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)
Comment 23•14 years ago
|
||
tryservered that patch and got green.
Assignee | ||
Comment 24•14 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•14 years ago
|
||
i'll check my other b-c changes in case they need an unbitrot.
Assignee | ||
Updated•14 years ago
|
Flags: in-testsuite?
Assignee | ||
Updated•14 years ago
|
Assignee | ||
Comment 26•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/9b9fa1cc982e
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
Assignee | ||
Updated•8 years ago
|
Flags: in-testsuite? → in-testsuite+
Updated•6 years ago
|
Component: BrowserTest → Mochitest
You need to log in
before you can comment on or make changes to this bug.
Description
•