Closed Bug 969437 Opened 6 years ago Closed 6 years ago

add some way to disable mochitest test summarization for locally-run tests

Categories

(Testing :: Mochitest, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla31

People

(Reporter: froydnj, Assigned: froydnj)

References

Details

Attachments

(2 files, 1 obsolete file)

Honza Bambas mentioned yesterday that being able to see all the TEST-PASS messages is a useful thing, because you can correlate what tests are passing with, e.g. NSPR logging output.  We should add some sort of capability of doing this, probably through mach (Honza mentioned he used |mach -v| and verboseness seems appropriate for this sort of thing).
+1 to this. See also bug 973000.
Duplicate of this bug: 973000
Andrew, do you plan bundle the fix for bug 973000 into this bug? They're two separate feature requests, which is why I filed it separately.
Flags: needinfo?(ahalberstadt)
I thought dup'ing the bugs was a little odd too, but no matter.

Here's a patch for the scenario Bobby described.  I don't feel great about
reaching into the guts of parentRunner, but I don't think there's another good
option.
Attachment #8376453 - Flags: feedback?(jmaher)
Attachment #8376453 - Flags: feedback?(bobbyholley)
Comment on attachment 8376453 [details] [diff] [review]
always output log messages for a single mochitest run

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

::: testing/mochitest/tests/SimpleTest/SimpleTest.js
@@ +22,5 @@
>  // In normal test runs, the window that has a TestRunner in its parent is
>  // the primary window.  In single test runs, if there is no parent and there
>  // is no opener then it is the primary window.
> +var isSingleTestRun = (parent == window && !opener)
> +var isPrimaryTestWindow = !!parent.TestRunner || isSingleTestRun;

this makes more sense now

@@ +322,5 @@
>          var outputCoalescedMessage = numCoalescedMessages == coalesceThreshold;
>  
> +        var runningSingleTest = ((parentRunner &&
> +                                  parentRunner._urls.length == 1)
> +                                 || isSingleTestRun);

can't we just say:
if runningSingleTest: outputCoalescedMessage = False
Attachment #8376453 - Flags: feedback?(jmaher) → feedback+
(In reply to Joel Maher (:jmaher) from comment #5)
> @@ +322,5 @@
> >          var outputCoalescedMessage = numCoalescedMessages == coalesceThreshold;
> >  
> > +        var runningSingleTest = ((parentRunner &&
> > +                                  parentRunner._urls.length == 1)
> > +                                 || isSingleTestRun);
> 
> can't we just say:
> if runningSingleTest: outputCoalescedMessage = False

I think you mean outputCoalescedMessage = True?

Simpler would be good, but outputCoalescedMessage == True causes us to append verbiage about elided messages, which would be inaccurate and misleading.
(In reply to Bobby Holley (:bholley) from comment #3)
> Andrew, do you plan bundle the fix for bug 973000 into this bug? They're two
> separate feature requests, which is why I filed it separately.

Sorry, I misread the title of this bug. I thought it said "Disable test summarization for locally run tests" instead of "Add some way...". Feel free to re-open the other bug if that doesn't get fixed here. (Once this is in place the other bug is just a simple change to the mach target).
Flags: needinfo?(ahalberstadt)
Blocks: 973000
Comment on attachment 8376453 [details] [diff] [review]
always output log messages for a single mochitest run

Works locally. Thanks for doing this!

Maybe this patch should move to bug 973000?
Attachment #8376453 - Flags: feedback?(bobbyholley) → feedback+
Comment on attachment 8376453 [details] [diff] [review]
always output log messages for a single mochitest run

This patch is no longer applicable to this bug.
Attachment #8376453 - Attachment is obsolete: true
Bill mentioned this to me on IRC the other day, too, also in the context of logging things across several tests.  He argued that any silencing of things should be opt-in, ala MOZ_QUIET.

The only problem is that I'm not sure how to satisfy conflicting desires here:

- automation wants as few messages as possible, given that B2G requires it to avoid timing out;
- (some?) local developers want as many messages as possible, to facilitate debugging.

Both of these paths go through automationutils et al, so we can't set anything there.  The easiest course I can see is to have a --log-all-messages flag (wordshedding welcome), but that's more typing for people.  Anybody else have ideas that I'm missing here?
Actually mochitests don't use automation.py/automationutils anymore. Some of the functionality that used to live there is now in runtests.py, but most of it is in the various components of mozbase.
Also, it would be really easy to opt-in to summarization in automation if there was a command line argument to do it. We'd just need to add it to the various mozharness scripts, e.g:

http://mxr.mozilla.org/build/source/mozharness/configs/unittests/linux_unittest.py#54
http://mxr.mozilla.org/build/source/mozharness/configs/b2g/emulator_automation_config.py#59
http://mxr.mozilla.org/build/source/mozharness/configs/android/androidarm.py#202
(In reply to Andrew Halberstadt [:ahal] from comment #12)
> Also, it would be really easy to opt-in to summarization in automation if
> there was a command line argument to do it. We'd just need to add it to the
> various mozharness scripts, e.g:
> 
> http://mxr.mozilla.org/build/source/mozharness/configs/unittests/
> linux_unittest.py#54
> http://mxr.mozilla.org/build/source/mozharness/configs/b2g/
> emulator_automation_config.py#59
> http://mxr.mozilla.org/build/source/mozharness/configs/android/androidarm.
> py#202

Ah, ok, didn't know about those.  Forgot that there are separate "run tests" for automation and developers.  Should we worry about those getting used for developers in Some Great Automation Unification down the road, or should we just cross that bridge when we come to it?  I guess we can just go ahead and do it and somebody will yell if it breaks down the road.
OK, here's a patch that adds --quiet and makes not-quiet (that is, print all
TEST-PASS, TEST-INFO, etc.) the default.

What's the magic for updating mozharness along with this?  I guess Andrew
hasn't backed out the B2G-only mochitest summarization bits, so landing this
and having it run without the mozharness bits (and/or riding the trains with
it?) won't cause anything to fall over, just run slightly slower...
Attachment #8387676 - Flags: review?(jmaher)
n.b. Mochitest still uses automationutils.py (I'd like to move that all into Mozbase at some point).
(In reply to Nathan Froyd (:froydnj) from comment #14)
> Created attachment 8387676 [details] [diff] [review]
> introduce a --quiet option for mochitests
> 
> OK, here's a patch that adds --quiet and makes not-quiet (that is, print all
> TEST-PASS, TEST-INFO, etc.) the default.
> 
> What's the magic for updating mozharness along with this?  I guess Andrew
> hasn't backed out the B2G-only mochitest summarization bits, so landing this
> and having it run without the mozharness bits (and/or riding the trains with
> it?) won't cause anything to fall over, just run slightly slower...

Unfortunately I did back it out. I'll whip up a mozharness patch that we can land before landing your patch that should prevent any bustage.
Bah, forgot this is going to be difficult to do since the --quiet command won't exist on older branches. We really need to get those mozharness configs in-tree :(.

I think our best options are to either:
A) land a dummy --quiet option that does nothing on all the older branches
B) switch back to the opt-out method and make the mach targets pass in the out-out by default
C) block on getting configs in mozharness and spend some time nailing this down

(In reply to Nathan Froyd (:froydnj) from comment #13)
> Should we worry about those getting used for
> developers in Some Great Automation Unification down the road, or should we
> just cross that bridge when we come to it?  I guess we can just go ahead and
> do it and somebody will yell if it breaks down the road.

Yeah, we eventually want to merge the mach/mozharness methods of running tests together. I would say let's cross that road when we come to it though.
I filed bug 981030 for option C.. but if this bug is time sensitive I wouldn't recommend blocking on it.
Comment on attachment 8387676 [details] [diff] [review]
introduce a --quiet option for mochitests

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

that is a lot of places to add code just for a single parameter to the test runner!
Attachment #8387676 - Flags: review?(jmaher) → review+
Bug 981030 is finished now!

You can just add --quiet to all of the 'mochitest_options' keys in these files:
http://mxr.mozilla.org/mozilla-central/source/testing/config/mozharness/

No need for landing on older branches anymore.
ahal++ for moving these things in-tree!  Your reward: reviewing a patch
to add --quiet to all these shiny new configs.
Attachment #8397238 - Flags: review?(ahalberstadt)
Comment on attachment 8397238 [details] [diff] [review]
part 2 - add --quiet option to all mochitest configurations

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

Lgtm :)
Attachment #8397238 - Flags: review?(ahalberstadt) → review+
Assignee: nobody → nfroyd
https://hg.mozilla.org/mozilla-central/rev/c7cecd865ea6
https://hg.mozilla.org/mozilla-central/rev/c8608d5e26ff
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.