Last Comment Bug 797827 - Make SimpleTest.is and variants less verbose on pass
: Make SimpleTest.is and variants less verbose on pass
Status: RESOLVED FIXED
:
Product: Testing
Classification: Components
Component: Mochitest (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla18
Assigned To: Simon Montagu :smontagu
:
Mentors:
: 797645 (view as bug list)
Depends on:
Blocks: logspam 798062
  Show dependency treegraph
 
Reported: 2012-10-04 05:48 PDT by Simon Montagu :smontagu
Modified: 2012-10-12 02:15 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed


Attachments
Don't output expected results on pass (1.88 KB, patch)
2012-10-04 05:48 PDT, Simon Montagu :smontagu
jmaher: review+
Details | Diff | Splinter Review

Description Simon Montagu :smontagu 2012-10-04 05:48:18 PDT
Created attachment 667925 [details] [diff] [review]
Don't output expected results on pass

A large percentage of mochitest logs consist of "foo should equal foo" (where foo is often quite lengthy) outputted by SimpleTest.is(). We certainly don't need the two identical copies of the test result, and I question whether we even need one when the test has passed.

A mochitest-4 log with this patch took up ~36MB, as opposed to ~50MB without the patch.
Comment 1 Joel Maher ( :jmaher ) 2012-10-04 06:10:02 PDT
Comment on attachment 667925 [details] [diff] [review]
Don't output expected results on pass

Review of attachment 667925 [details] [diff] [review]:
-----------------------------------------------------------------

Is there anyway we could make this configurable?  It is nice to know that we are passing something and by removing the description of what we are testing, the output becomes useless.  What is of most concern in a log file is finding what went wrong and obtaining enough information from the log itself to diagnose the problem.  Since this patch allows us to do that, I am fine going with an r+.   

We should ask ourselves if we need to output anything on a pass (even the "XXXXXX TEST-PASS | ...") as we could really trim up our logs if needed.  A slippery slope and making it configurable would help ensure we are not forgetting about the pros and cons of doing this.

As a bonus this should make our IO for android testing less which is a really good thing!
Comment 2 Andrew McCreight [:mccr8] 2012-10-04 07:43:47 PDT
*** Bug 797645 has been marked as a duplicate of this bug. ***
Comment 3 Daniel Holbert [:dholbert] 2012-10-04 09:21:03 PDT
(In reply to Joel Maher (:jmaher) from comment #1)
> We should ask ourselves if we need to output anything on a pass (even the
> "XXXXXX TEST-PASS | ...") as we could really trim up our logs if needed.

FWIW -- in my experience, the TEST-PASS lines make it easier to establish the context for crashes/hangs, so I'd advocate against removing TEST-PASS lines entirely.

e.g. suppose a test tests something on all of our CSS properties, and when it gets to a particular one, we occasionally will hang/crash.  In the logs that experience the hang/crash, it's much more useful to see something like

>  TEST-INFO | starting test test_allTheThings.html
>  [...]
>  TEST-PASS | property 'n' behaved like we expected
> ***CRASH*** (or hang/no more output/whatever)

...rather than just...

>  TEST-INFO | starting test test_allTheThings.html
> ***CRASH*** (or hang/no more output/whatever)
Comment 4 Daniel Holbert [:dholbert] 2012-10-04 09:24:27 PDT
(in that case, property 'n+1' would presumably be the buggy one)

I suppose if the TEST-PASS presence/complete-absence were configurable, we could turn it off in the majority of cases, and then turn it back on eagerly if we need it.  The downside is, our output would be mysterious (like the latter chunk in comment 3) the first time we hit a sporadic failure, and we'd need someone to take action (configure it on) in order to figure out more... but maybe that's worth it for reduced log-sizes.
Comment 5 Joel Maher ( :jmaher ) 2012-10-04 09:25:43 PDT
dholbert++
Comment 7 Ed Morley [:emorley] 2012-10-05 04:02:39 PDT
https://hg.mozilla.org/mozilla-central/rev/d30e007d711f
Comment 8 Ed Morley [:emorley] 2012-10-12 02:15:28 PDT
Landed on mozilla17 with the blanket a=test-only to reduce log size for esr17, which will be with us until December 2013 (yey):
https://hg.mozilla.org/releases/mozilla-beta/rev/e94f91b6473c

Note You need to log in before you can comment on or make changes to this bug.