Hide big table of tests while tests are running in harness

RESOLVED FIXED

Status

defect
RESOLVED FIXED
11 years ago
8 years ago

People

(Reporter: bzbarsky, Assigned: heycam)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [buildfaster:p1])

Attachments

(2 attachments, 7 obsolete attachments)

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?
I think we may as well hide the table for a --close-when-done run, which is what matters for tinderbox, at least.
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).
(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.
Posted patch Patch (v1) (obsolete) β€” β€” Splinter Review
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)
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
Attachment #373108 - Flags: review?(ted.mielczarek) → review?(jwalden+bmo)
Comment on attachment 373108 [details] [diff] [review]
Patch (v1)

Ted pointed out that Waldo might be a better reviewer here.
(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.)
Posted patch Patch (v2) (obsolete) β€” β€” Splinter Review
(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)
> they can click "Show Results Table"

Generally speaking, that will cause test failures.
(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.
If we had a mode that only showed the failures, that would be ideal for me.
(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.)
Attachment #373276 - Flags: review?(dbaron)
Comment on attachment 373276 [details] [diff] [review]
Patch (v2)

Review, anyone?
Attachment #373276 - Flags: review?(jwalden+bmo)
Attachment #373276 - Flags: review?(dbaron)
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.  :-)
Attachment #373276 - Attachment is obsolete: true
Whiteboard: [needs new patch]
Updating to reality: I won't have the time to work on this for the foreseeable future!
Assignee: ehsan.akhgari → nobody
Status: ASSIGNED → NEW
Whiteboard: [needs new patch] → [needs new patch][buildfaster:p1]
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
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)
Attachment #545062 - Flags: review?(catlee)
(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.
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.
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.
(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?
Attachment #545061 - Attachment is obsolete: true
Attachment #545111 - Flags: review?(ted.mielczarek)
Attachment #545061 - Flags: review?(ted.mielczarek)
Attachment #545062 - Attachment is obsolete: true
Attachment #545112 - Flags: review?(catlee)
Attachment #545062 - Flags: review?(catlee)
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).
I am curious why we are setting an environment variable rather than passing in a command line argument?
oh- nm, forgot it was mentioned a couple comments earlier
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 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-
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?
Attachment #545576 - Flags: review? → review?(jmaher)
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)
Attachment #545577 - Flags: review?(catlee) → review+
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+
http://hg.mozilla.org/build/buildbotcustom/rev/4b59cdc14898

Gonna wait until that one's in production before landing the other part.
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).
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.
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.
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
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
I'm not able to reproduce the hang locally with the Tinderbox build.
Target Milestone: mozilla8 → ---
does it reproduce on try server?
Yes.  I'll try bisecting the set of tests run in mochitest-1 in case it's a particular test that causes the hang.
No luck, splitting m-1 in half and running each half independently hides the hang again.
Duplicate of this bug: 673539
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 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+
Nice work, jgriffin!
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
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: 8 years ago
Resolution: --- → FIXED
Do we know how much time has been saved with this change?
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.