Closed
Bug 479352
Opened 16 years ago
Closed 13 years ago
Hide big table of tests while tests are running in harness
Categories
(Testing :: Mochitest, defect)
Testing
Mochitest
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: bzbarsky, Assigned: heycam)
References
Details
(Whiteboard: [buildfaster:p1])
Attachments
(2 files, 7 obsolete files)
(deleted),
patch
|
catlee
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jmaher
:
review+
|
Details | Diff | Splinter Review |
Bug 467672 added a bunch of tests that change the preferences for how numbers should render. Sadly, each preference change means we have to do a full reflow of the entire mochitest harness page. This is a huge table, and the reflows take a while (measured in tens of seconds _each_ in a debug build on my machine). The test does 40+ such reflows. Running through that set of tests over here took over 30 minutes, easily.
Could we at least hide the huge table while running these tests? Or something? Or just generally hide it while doing an auto run?
Comment 1•16 years ago
|
||
I think we may as well hide the table for a --close-when-done run, which is what matters for tinderbox, at least.
Reporter | ||
Comment 2•16 years ago
|
||
It's almost never what matters for me, because I want to investigate the failures when done (if any).
So it'd be nice if we could hide it in general (and maybe show on the first failure or at end of test or something).
Comment 3•16 years ago
|
||
(In reply to comment #1)
> I think we may as well hide the table for a --close-when-done run, which is
> what matters for tinderbox, at least.
This seems very reasonable, since you can't possibly look at the table if we close the window when we finish.
Comment 4•16 years ago
|
||
So, this patch does the following:
* The test results table is hidden by default in all test runs.
* There is a link to toggle the visibility of the table which is available in all runs.
* If the suite is not started with --close-when-done, upon hitting the first failure, the results table is displayed, and remains displayed until the end of the test run unless manually hidden.
* Three new SimpleTest APIs were added to be used by tests which always want the results table to be hidden for performance reasons. For example, the tests for bug 467672 could use this. I'm planning to make them use it in bug 488553.
This patch affects mochitest-plain, mochitest-chrome and mochitest-a11y.
Assignee: nobody → ehsan.akhgari
Status: NEW → ASSIGNED
Attachment #373108 -
Flags: review?(ted.mielczarek)
Updated•16 years ago
|
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
Updated•16 years ago
|
Attachment #373108 -
Flags: review?(ted.mielczarek) → review?(jwalden+bmo)
Comment 5•16 years ago
|
||
Comment on attachment 373108 [details] [diff] [review]
Patch (v1)
Ted pointed out that Waldo might be a better reviewer here.
Comment 6•16 years ago
|
||
(In reply to comment #4)
> * If the suite is not started with --close-when-done, upon hitting the first
> failure, the results table is displayed, and remains displayed until the end of
> the test run unless manually hidden.
I don't understand what the advantage is to show the table while the test suite is still running.
I would rather do it at the end of the run.
*****
An additional feature, if possible, would be to be able to filter out the all-green lines. Or maybe a three states (loop): red only, red+orange, all.
(I write it here/now as a reminder but it could be done in a different patch/bug.)
Comment 7•16 years ago
|
||
(In reply to comment #6)
> I don't understand what the advantage is to show the table while the test suite
> is still running.
> I would rather do it at the end of the run.
I thought about it a bit, and I think you're right. If someone wants to see the results table during the middle of test runs, they can click "Show Results Table" at any time. Otherwise it will appear at the end of the run if close when done is not set.
Boris, do you agree?
> An additional feature, if possible, would be to be able to filter out the
> all-green lines. Or maybe a three states (loop): red only, red+orange, all.
> (I write it here/now as a reminder but it could be done in a different
> patch/bug.)
I think it's possible, but can you please file a bug on that? I'll give it a shot so you can assign it to me. :-)
Attachment #373108 -
Attachment is obsolete: true
Attachment #373276 -
Flags: review?(jwalden+bmo)
Attachment #373108 -
Flags: review?(jwalden+bmo)
Reporter | ||
Comment 8•16 years ago
|
||
> they can click "Show Results Table"
Generally speaking, that will cause test failures.
Comment 9•16 years ago
|
||
(In reply to comment #8)
> > they can click "Show Results Table"
>
> Generally speaking, that will cause test failures.
Hmm, you're right. So do you prefer the previous approach of "show on first failure"? One can't really scroll to the results while the tests are running FWIW.
Reporter | ||
Comment 10•16 years ago
|
||
If we had a mode that only showed the failures, that would be ideal for me.
Comment 11•16 years ago
|
||
(In reply to comment #7)
> can you please file a bug on that?
Bug 483407 will be good for this too ;-)
(In reply to comment #9)
> So do you prefer the previous approach of "show on first failure"?
No, only the top of the table is visible anyway...
(In reply to comment #10)
> If we had a mode that only showed the failures, that would be ideal for me.
I concur ... as this is the interesting part and has a chance to stay on the visible area. (See comment 6.)
Updated•16 years ago
|
Attachment #373276 -
Flags: review?(dbaron)
Comment 12•16 years ago
|
||
Comment on attachment 373276 [details] [diff] [review]
Patch (v2)
Review, anyone?
Updated•16 years ago
|
Attachment #373276 -
Flags: review?(jwalden+bmo)
Attachment #373276 -
Flags: review?(dbaron)
Comment 13•16 years ago
|
||
Comment on attachment 373276 [details] [diff] [review]
Patch (v2)
Actually I've been working on another version of this patch which implements comment 10. It's not quite ready for review, but let's hold off on the review of this patch because soon it'll be moot. :-)
Updated•16 years ago
|
Attachment #373276 -
Attachment is obsolete: true
Updated•16 years ago
|
Whiteboard: [needs new patch]
Comment 14•15 years ago
|
||
Updating to reality: I won't have the time to work on this for the foreseeable future!
Assignee: ehsan.akhgari → nobody
Status: ASSIGNED → NEW
Assignee | ||
Comment 15•13 years ago
|
||
I'll take this. I'm not sure tying the hiding of the results table to whether --close-when-done was used is the right thing. How about as a first step, before improving the current results display to show only failures (for example) that we add a --hide-results-table option to runtests.py and have the buildbot steps always use that?
Assignee: nobody → cam
Status: NEW → ASSIGNED
Assignee | ||
Comment 16•13 years ago
|
||
This hides the results table if --hide-results-table is passed to runtests.py. The make target doesn't, by default. A separate patch will make the build machines use this command line argument.
Attachment #545061 -
Flags: review?(ted.mielczarek)
Assignee | ||
Comment 17•13 years ago
|
||
Attachment #545062 -
Flags: review?(catlee)
Comment 18•13 years ago
|
||
(In reply to comment #15)
> I'll take this. I'm not sure tying the hiding of the results table to
> whether --close-when-done was used is the right thing. How about as a first
> step, before improving the current results display to show only failures
> (for example) that we add a --hide-results-table option to runtests.py and
> have the buildbot steps always use that?
Will older runtests.py fail if this new flag is passed to it? If so, we'd have to land this on all active branches first, or have a flag that tells buildbot to use --hide-results-table on some branches but not on others.
Assignee | ||
Comment 19•13 years ago
|
||
You're right, it will fail with the unrecognised flag. Is it acceptable to land the runtests.py changes on all the active branches? If not, we could invert the meaning of the flag -- use --show-results-table instead, have the makefile targets use this, and require people running runtests.py manually to specify the flag.
Assignee | ||
Comment 20•13 years ago
|
||
A safer alternative then could be to have the buildbot step set an environment variable indicating that we're building on the build farm. Then we could land the hide-the-table patch only on the branches we care about.
Comment 21•13 years ago
|
||
(In reply to comment #20)
> A safer alternative then could be to have the buildbot step set an
> environment variable indicating that we're building on the build farm. Then
> we could land the hide-the-table patch only on the branches we care about.
Shouldn't be too hard to do....MOZ_HIDE_RESULTS_TABLE=1?
Assignee | ||
Comment 22•13 years ago
|
||
Attachment #545061 -
Attachment is obsolete: true
Attachment #545111 -
Flags: review?(ted.mielczarek)
Attachment #545061 -
Flags: review?(ted.mielczarek)
Assignee | ||
Comment 23•13 years ago
|
||
Attachment #545062 -
Attachment is obsolete: true
Attachment #545112 -
Flags: review?(catlee)
Attachment #545062 -
Flags: review?(catlee)
Comment 24•13 years ago
|
||
Bear, Joel,
Let's be sure this environment variable is set on mobile testing too. I think it should help make tegras a bit more stable. (it takes a bunch of memory to draw/update that table on fennec).
Comment 25•13 years ago
|
||
I am curious why we are setting an environment variable rather than passing in a command line argument?
Comment 26•13 years ago
|
||
oh- nm, forgot it was mentioned a couple comments earlier
Comment 27•13 years ago
|
||
Comment on attachment 545111 [details] [diff] [review]
Hide mochitest results table if MOZ_HIDE_RESULTS_TABLE=1 is set. (v2)
Review of attachment 545111 [details] [diff] [review]:
-----------------------------------------------------------------
this looks good to me. Ted is on PTO for the week, so I wanted to pick up a few reviews of his. I tested this locally and didn't see any problems. If you would rather get a review from :ted, please ask for it again and I will leave this alone:)
Attachment #545111 -
Flags: review?(ted.mielczarek) → review+
Comment 28•13 years ago
|
||
Comment on attachment 545112 [details] [diff] [review]
Set MOZ_HIDE_RESULTS_TABLE=1 when running mochitests. (v2)
I think this should go into env.py instead.
Attachment #545112 -
Flags: review?(catlee) → review-
Assignee | ||
Comment 29•13 years ago
|
||
I've changed the patch a bit so that it doesn't look up os.environ for MOZ_HIDE_RESULTS_TABLE but instead the result of the Mochitest.environment() call. That, along with the small change in remoteautomation.py's environment() function should allow the variable to propagate over to the remote side for Android. This part I couldn't test since I don't have a local Android environment set up -- jmaher can you verify it'll work?
Attachment #545111 -
Attachment is obsolete: true
Attachment #545576 -
Flags: review?
Assignee | ||
Updated•13 years ago
|
Attachment #545576 -
Flags: review? → review?(jmaher)
Assignee | ||
Comment 30•13 years ago
|
||
Set it from env.py. (Flying blind here, so please verify this'll work! I think I got all the environments that are relevant.)
Attachment #545112 -
Attachment is obsolete: true
Attachment #545577 -
Flags: review?(catlee)
Updated•13 years ago
|
Attachment #545577 -
Flags: review?(catlee) → review+
Comment 31•13 years ago
|
||
Comment on attachment 545576 [details] [diff] [review]
Hide mochitest results table if MOZ_HIDE_RESULTS_TABLE=1 is set. (v3)
Review of attachment 545576 [details] [diff] [review]:
-----------------------------------------------------------------
runs as expected on android. updated patch looks good.
Attachment #545576 -
Flags: review?(jmaher) → review+
Assignee | ||
Comment 32•13 years ago
|
||
http://hg.mozilla.org/build/buildbotcustom/rev/4b59cdc14898
Gonna wait until that one's in production before landing the other part.
Assignee | ||
Comment 33•13 years ago
|
||
It looks like it's in production now (checking a recent desktop Mochitest build shows the variable being set), but it doesn't seem to have worked for Android tests (e.g. http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1310772857.1310773389.12502.gz&fulltext=1).
Comment 34•13 years ago
|
||
it could be that the "MOZ_HIDE_RESULTS_TABLE" isn't set for the tools that run the android tests. I know the patch indicated that, but we should double check that. Maybe :bear would be able to help decipher the maze of buildbot on android.
Assignee | ||
Comment 35•13 years ago
|
||
Oh, actually I need to land other half of the patch -- that modifies build/mobile/remoteautomation.py to ensure the environment variable gets propagated down to the device.
Assignee | ||
Comment 36•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/48dcb60519ac
Should we file a followup bug for the "only show failures in the table" behaviour?
Whiteboard: [needs new patch][buildfaster:p1] → [buildfaster:p1]
Target Milestone: --- → mozilla8
Comment 37•13 years ago
|
||
Backed that out because it seemed to have introduced Mochitest1 hangs on Linux64 debug (odd, but seems relatively consistent):
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1310862028.1310864289.13030.gz
http://hg.mozilla.org/mozilla-central/rev/da362a07a5ee
Assignee | ||
Comment 38•13 years ago
|
||
I'm not able to reproduce the hang locally with the Tinderbox build.
Target Milestone: mozilla8 → ---
Comment 39•13 years ago
|
||
does it reproduce on try server?
Assignee | ||
Comment 40•13 years ago
|
||
Yes. I'll try bisecting the set of tests run in mochitest-1 in case it's a particular test that causes the hang.
Assignee | ||
Comment 41•13 years ago
|
||
No luck, splitting m-1 in half and running each half independently hides the hang again.
Comment 43•13 years ago
|
||
This patch hides the results table in a different way; ie adds the "invisible" class to the test-table itself, rather than hiding the entire test listing document.
I've run this 3 times on try with mochitest-1 and have had zero oranges; I'm going to run it again now with against all mochitests.
Attachment #545576 -
Attachment is obsolete: true
Attachment #549159 -
Flags: review?(jmaher)
Comment 44•13 years ago
|
||
Comment on attachment 549159 [details] [diff] [review]
Hide mochitest results table if MOZ_HIDE_RESULTS_TABLE=1 is set. (v4)
Review of attachment 549159 [details] [diff] [review]:
-----------------------------------------------------------------
assuming this passes all mochitests on try, we should be good.
r=me with the harness.css cleanup done.
::: testing/mochitest/static/harness.css
@@ +51,5 @@
>
> +.hide-results-table #test-table {
> + display: none;
> +}
> +
please remove, we are not using this anymore
Attachment #549159 -
Flags: review?(jmaher) → review+
Assignee | ||
Comment 45•13 years ago
|
||
Nice work, jgriffin!
Comment 46•13 years ago
|
||
The try runs for v4 all came back green, so hopefully we're good. I just pushed this latest version to m-c:
http://hg.mozilla.org/mozilla-central/rev/e9be8677a7ae
Comment 47•13 years ago
|
||
This landed, so I don't see any need to keep the bug open any longer. Moving to FIXED, please reopen if it should remain open.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Comment 48•13 years ago
|
||
Do we know how much time has been saved with this change?
Assignee | ||
Comment 49•13 years ago
|
||
Here are the times for mochitests for the build just before and just after it landed: http://mcc.id.au/temp/2011/times2.html
You need to log in
before you can comment on or make changes to this bug.
Description
•