479352 - Hide big table of tests while tests are running in harness

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Boris Zbarsky [:bzbarsky]]

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

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think we may as well hide the table for a --close-when-done run, which is what matters for tinderbox, at least.

Comment 2

[Boris Zbarsky [:bzbarsky]]

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

[(not currently active) Ted Mielczarek]

(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

[(no longer active)]

Attachment: Patch (v1)

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.

Comment 5

[(no longer active)]

Comment on attachment 373108 [details] [diff] [review]
Patch (v1)

Ted pointed out that Waldo might be a better reviewer here.

Comment 6

[Serge Gautherie (:sgautherie)]

(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

[(no longer active)]

Attachment: Patch (v2)

(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.  :-)

Comment 8

[Boris Zbarsky [:bzbarsky]]

> they can click "Show Results Table"

Generally speaking, that will cause test failures.

Comment 9

[(no longer active)]

(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.

Comment 10

[Boris Zbarsky [:bzbarsky]]

If we had a mode that only showed the failures, that would be ideal for me.

Comment 11

[Serge Gautherie (:sgautherie)]

(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.)

Comment 12

[Serge Gautherie (:sgautherie)]

Comment on attachment 373276 [details] [diff] [review]
Patch (v2)

Review, anyone?

Comment 13

[(no longer active)]

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.  :-)

Comment 14

[(no longer active)]

Updating to reality: I won't have the time to work on this for the foreseeable future!

Comment 15

[Cameron McCormack (:heycam)]

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?

Comment 16

[Cameron McCormack (:heycam)]

Attachment: Hide mochitest results table if --hide-results-table is passed to runtests.py.

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.

Comment 17

[Cameron McCormack (:heycam)]

Attachment: Pass --hide-results-table to runtests.py.

Comment 18

[Chris AtLee [: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.

Comment 19

[Cameron McCormack (:heycam)]

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.

Comment 20

[Cameron McCormack (:heycam)]

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

[Chris AtLee [:catlee]]

(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?

Comment 22

[Cameron McCormack (:heycam)]

Attachment: Hide mochitest results table if MOZ_HIDE_RESULTS_TABLE=1 is set. (v2)

Comment 23

[Cameron McCormack (:heycam)]

Attachment: Set MOZ_HIDE_RESULTS_TABLE=1 when running mochitests. (v2)

Comment 24

[cmtalbert]

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

[Joel Maher ( :jmaher ) (UTC -8)]

I am curious why we are setting an environment variable rather than passing in a command line argument?

Comment 26

[Joel Maher ( :jmaher ) (UTC -8)]

oh- nm, forgot it was mentioned a couple comments earlier

Comment 27

[Joel Maher ( :jmaher ) (UTC -8)]

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:)

Comment 28

[Chris AtLee [:catlee]]

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.

Comment 29

[Cameron McCormack (:heycam)]

Attachment: Hide mochitest results table if MOZ_HIDE_RESULTS_TABLE=1 is set. (v3)

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?

Comment 30

[Cameron McCormack (:heycam)]

Attachment: Set MOZ_HIDE_RESULTS_TABLE=1 when running mochitests. (v3)

Set it from env.py.  (Flying blind here, so please verify this'll work!  I think I got all the environments that are relevant.)

Comment 31

[Joel Maher ( :jmaher ) (UTC -8)]

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.

Comment 32

[Cameron McCormack (:heycam)]

http://hg.mozilla.org/build/buildbotcustom/rev/4b59cdc14898

Gonna wait until that one's in production before landing the other part.

Comment 33

[Cameron McCormack (:heycam)]

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

[Joel Maher ( :jmaher ) (UTC -8)]

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.

Comment 35

[Cameron McCormack (:heycam)]

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.

Comment 36

[Cameron McCormack (:heycam)]

http://hg.mozilla.org/mozilla-central/rev/48dcb60519ac

Should we file a followup bug for the "only show failures in the table" behaviour?

Comment 37

[:Gavin Sharp [email: [e-mail]]]

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

Comment 38

[Cameron McCormack (:heycam)]

I'm not able to reproduce the hang locally with the Tinderbox build.

Comment 39

[Joel Maher ( :jmaher ) (UTC -8)]

does it reproduce on try server?

Comment 40

[Cameron McCormack (:heycam)]

Yes.  I'll try bisecting the set of tests run in mochitest-1 in case it's a particular test that causes the hang.

Comment 41

[Cameron McCormack (:heycam)]

No luck, splitting m-1 in half and running each half independently hides the hang again.

Comment 43

[Jonathan Griffin (:jgriffin)]

Attachment: Hide mochitest results table if MOZ_HIDE_RESULTS_TABLE=1 is set. (v4)

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.

Comment 44

[Joel Maher ( :jmaher ) (UTC -8)]

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

Comment 45

[Cameron McCormack (:heycam)]

Nice work, jgriffin!

Comment 46

[Jonathan Griffin (: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

Comment 47

[cmtalbert]

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.

Comment 48

[Mounir Lamouri (:mounir)]

Do we know how much time has been saved with this change?

Comment 49

[Cameron McCormack (:heycam)]

Here are the times for mochitests for the build just before and just after it landed: http://mcc.id.au/temp/2011/times2.html