1020625 - structured logging's suite_end should not be called by moztest unittest adapter

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Chris Manchester (limited bugmail, email directly)]

There's a mismatch in expectations between moztests unittest adapter, the unittest formatter, and the way marionette currently uses the unittest runner class. Marionette's use of a runner class results in suite_start and suite_end being called for each file. The state of the formatter persists between calls to suite_end, meaning the output is... weird. Particularly, a summary is printed after each file runs and any failure from a previously run file will be included.

suite_end should be called only once per run, not once per file. In the case of marionette, the marionette runner should call suite_end rather than the runner class. My original thought was to make startTestRun and stopTestRun in moztest.adapters.unit.StructuredTestResult no-ops and leave this part up to the caller, but then I noticed Bug 1017596.

Should I revert the change in bug 1017596, or perhaps use a TestResult class specific to marionette?

Comment 1

[Andreas Tolfsen ❲:ato❳]

The intention of bug 1017596 was to send only one each of those events.  This appeared fine when tested against the semiauto test runner.  But because that puts all tests in a single unittest.TestSuite it's possible Marionette differs in that regard.

I can't say off the top of my head what the default unittest construction is, but will have to check.  I'm guessing that's the pattern we want to use for Marionette?

Comment 2

[Chris Manchester (limited bugmail, email directly)]

Marionette creates a new suite object for each file here, and runs each as a "run":

http://hg.mozilla.org/mozilla-central/annotate/882826199076/testing/marionette/client/marionette/runner/base.py#l971

Perhaps we could make it use the same suite for each file. It's not clear to me what the advantages or disadvantages to that might be.

According to the unittest documentation (and the design of Kent Beck's original framework in SmallTalk), a test suite may contain other test suites. Any time this is invoked, running sub-suites will end up running afoul of the expectations of mozlog.structured.

Comment 3

[James Graham [:jgraham]]

Yes, I think that mozlog should simply ignore the TestSuite structure and always present a single stream of tests and results. I don't think working around this in marionette (e.g. by consolidating all tests into the same TestSuite) is the right thing to do, because it will likely still be broken for other unittest-using test suites. Therefore we should first revert bug 1017596 and then, if possible, move the calls to stuite_start/suite_end up a bit so they are invoked once per TestRunner rather than once per TestSuite.

Comment 4

[Chris Manchester (limited bugmail, email directly)]

Attachment: Don't call structured logging's suite_start and suite_end per test suite

I'm convinced.

The meaning of suite_end differs between mozlog's structured logging 
protocol and that of unittest. This opts to leave behind that of unittest 
and favor mozlog's.

The main difference when converting from previous uses of unittest will be 
that we used to get a summary (timing, failure list, etc.) after each file, 
and we will now only get an aggregate at the very end.

I don't see any current users of moztest.adapters.unit.StructuredTestResult 
searching m-c. Are there others, potentially?

Comment 5

[James Graham [:jgraham]]

Comment on attachment 8435011 [details] [diff] [review]
Don't call structured logging's suite_start and suite_end per test suite

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

::: testing/mozbase/moztest/moztest/adapters/unit.py
@@ +32,5 @@
>                  debug_info.update(info)
>          return debug_info
>  
>      def startTestRun(self):
> +        pass

Can we add a comment here (and below) to indicate why we can't do exactly what we did?

I also wonder if we can safely move the code to StructuredTestRunner.run, or not.

Comment 6

[Andreas Tolfsen ❲:ato❳]

For the record I think avoiding the concept of unittest.TestSuite altogether is a wise move.  To properly serialize such a construct in the Firefox OS certification test suite we are forced to flatten the nested structure, because as :cmanchester mentions it can in theory contain instances of more TestSuite's (and due to inexplicable reasons unittest.defaultTestLoader.loadTestsFromName seems to be overly generous with empty nested structures).

Comment 7

[Chris Manchester (limited bugmail, email directly)]

> I also wonder if we can safely move the code to StructuredTestRunner.run, or
> not.

Marionette, at least, calls this once per file, so no. suite_start will have to be called before all calls to run, suite_end will have to be called after all calls to run.

So I don't break certsuite's output, should this be a version bump?

Comment 8

[Chris Manchester (limited bugmail, email directly)]

Attachment: Don't call structured logging's suite_start and suite_end per test suite

Added a comment to explain this change. As I understand it, to make this change
I should pin certsuite to the current version of moztest, bump the version with
this bug, then change certsuite to be compatible with this change and use the
new version. Is that about right?

Comment 9

[Andreas Tolfsen ❲:ato❳]

Yes, the version of moztest must be pinned in fxos-certsuite before this change.  The setup.py file listing moztest is being moved in https://critic.hoppipolla.co.uk/r/1756 so best to wait for that to land first.

Comment 10

[James Graham [:jgraham]]

I just pushed a change to the certsuite to pin the version number of mozlog, so we should be able to land and then update without fear of bustage.

Comment 11

[James Graham [:jgraham]]

Uh, but I should have pinned moztest. Bah.

Comment 12

[James Graham [:jgraham]]

Comment on attachment 8435812 [details] [diff] [review]
Don't call structured logging's suite_start and suite_end per test suite

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

Need to version bump moztest since this is backwards incompatible, but r+ with that change.

Comment 13

[Chris Manchester (limited bugmail, email directly)]

Attachment: Don't call structured logging's suite_start and suite_end per test suite

Patch with version bump.

Comment 14

[Chris Manchester (limited bugmail, email directly)]

Setting checkin-needed without a try run because this code is not yet used in tree, and the out of tree consumer has been modified to depend on a previous version of the package.

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/af526a65b9af

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/af526a65b9af