Closed Bug 1119952 Opened 5 years ago Closed 5 years ago

Suppress log spam when SimpleTest.finish() is called multiple times

Categories

(Testing :: Mochitest, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: mt, Assigned: mt)

Details

Attachments

(2 files, 1 obsolete file)

A number of times, tests have fatal error conditions that cause them to terminate early.  It's occasionally the case that a failure doesn't stop the test from proceeding, which results in lots more errors.

Counting the calls to finish() and suppressing those beyond ~5 or more helps keep things manageable.
Attached file MozReview Request: bz://1119952/mt (obsolete) —
Attachment #8546853 - Flags: review?(jmaher)
/r/2313 - Bug 1119952 - Reduce log spam from calling finished() multiple times, r=jmaher

Pull down this commit:

hg pull review -r da6ae100d165a681a17855fa563143be93118d61
https://reviewboard.mozilla.org/r/2311/#review1491

::: testing/mochitest/tests/SimpleTest/TestRunner.js
(Diff revision 1)
> +  if (!$('current-test-path')) {
> +    return;
> +  }

I neglected to look at this before; I think that I can be a little less aggressive here.
/r/2313 - Bug 1119952 - Reduce log spam from calling finished() multiple times, r=jmaher

Pull down this commit:

hg pull review -r de29e56b07f16039d5d4400cff2ee28a24cde9f7
Attachment #8546853 - Flags: review?(jmaher) → review+
Keywords: checkin-needed
Casting a wide net:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=425f51ef650c

Just for the linux debug failures:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4f693ec4ad43

I'll re-trigger the latter one a few times to see if it comes up again before asking to re-land.
Comment on attachment 8546853 [details]
MozReview Request: bz://1119952/mt

:jmaher, can you have a quick look at the updated patch to double-check my fixes?  I think that they are OK, but I don't want this backed out a second time.
Attachment #8546853 - Flags: review+ → review?(jmaher)
Comment on attachment 8546853 [details]
MozReview Request: bz://1119952/mt

:jmaher, can you sanity check the fixes please?  I think that they are OK, but I want to be extra careful to avoid another backout.
Comment on attachment 8546853 [details]
MozReview Request: bz://1119952/mt

sorry for not catching that in the first review.
Attachment #8546853 - Flags: review?(jmaher) → review+
Attachment #8546853 - Flags: review+ → review?(jmaher)
/r/2313 - Bug 1119952 - Reduce log spam from calling finished() multiple times, r=jmaher
/r/2723 - Bug 1119952 - Removing log-after-finish errors

Pull down these commits:

hg pull review -r 1bc26cf3ad8c0d80c2e8d1766c22073c0549486c
https://reviewboard.mozilla.org/r/2723/#review1811

::: testing/mochitest/tests/Harness_sanity/test_sanitySimpletest.html
(Diff revision 1)
> -          //expect and throw exception here. Otherwise, any code that follows the throw call will never be executed
> +          dump("Profile::SimpleTestRunTime: " + (endTime-startTime) + "\n");

why is this changed from info -> dump?

::: testing/mochitest/tests/Harness_sanity/test_sanitySimpletest.html
(Diff revision 1)
> +      SimpleTest.executeSoon(

I don't understand why we are calling executeSoon again, the first time we though an uncaught excpetion, then we call simpletest finish in the next call.

I am really not understanding this change.
Test runs are all fine, except...

:jmaher Just to follow-up on the review request.

The main problem with the fix is that the profiling output won't hit the structured logger.  That might be a problem if there is a consumer somewhere that relies on it.  The string doesn't appear anywhere else in the tree.  Are you aware of anyone using this profiling information?
which string are you concerned about?
The "Profile::SimpleTestRunTime" is structured in a way that suggests it is consumed by some tool.  I found no evidence of that tool, but don't want to break it if it exists.
Attachment #8546853 - Flags: review?(jmaher) → review-
I am not aware of anyone using Profile::SimpleTestRunTime.
Attachment #8546853 - Attachment is obsolete: true
Attachment #8619096 - Flags: review-
Attachment #8619097 - Flags: review-
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.