952530 - List all skipped/disabled tests rather than just the conditionally skipped tests

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Currently we output the tests conditionally skipped in the manifest files. Ideally we would list all skipped/disabled tests along with the reason.

Comment 1

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: List all skipped/disabled tests. v1.0

This patch will list all disabled tests along with the reason. If a reason is not in the manifest, but the test is conditionally skipped then the condition is the reason. Also, this will output the tests filtered out using the --type command line argument.

We should probably also include all of these tests as skipping in the results, so they appear in the HTML/XML reports. This is why I'm asking for feedback rather than review.

Comment 2

[Jonathan Griffin (:jgriffin)]

Comment on attachment 8350673 [details] [diff] [review]
List all skipped/disabled tests. v1.0

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

I agree with including the skipped tests in XML/HTML output.

Note this will likely be bitrotted by (or bitrot) bug 926277.

Comment 3

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: List all skipped/disabled tests. v1.1

I can't see how we'll get the full list of skipped tests in the HTML report if we never add them to the suite. Do you have any suggestions Jonathan? Unless the answer is simple, I would suggest we take care of this in a separate bug. What do you think?

Comment 4

[Jonathan Griffin (:jgriffin)]

Comment on attachment 8365900 [details] [diff] [review]
List all skipped/disabled tests. v1.1

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

As far as including the disabled tests in HTML reports, we could simply add the disabled tests to a self.disabled_tests list, and then use that list inside HTMLReportingTestRunnerMixin.

::: testing/marionette/client/marionette/runner/base.py
@@ +818,5 @@
> +                                  set([x['path'] for x in target_tests]))
> +            for test in filtered_tests:
> +                self.logger.info('TEST-SKIP | %s | filtered by type (%s)' % (
> +                    os.path.basename(test),
> +                    self.type))

So, the code starting on line 809 prints out the disabled tests; the code here prints out tests that were filtered out by --type args.  But, I don't see the equivalent anywhere of the original code which prints out tests that were disabled due to device type (e.g., skipped due to being a b2g desktop build).  Am I missing something?

Comment 5

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Jonathan Griffin (:jgriffin) from comment #4)
> Comment on attachment 8365900 [details] [diff] [review]
> List all skipped/disabled tests. v1.1
> 
> Review of attachment 8365900 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> As far as including the disabled tests in HTML reports, we could simply add
> the disabled tests to a self.disabled_tests list, and then use that list
> inside HTMLReportingTestRunnerMixin.

Sounds straight-forward enough. :)

> ::: testing/marionette/client/marionette/runner/base.py
> @@ +818,5 @@
> > +                                  set([x['path'] for x in target_tests]))
> > +            for test in filtered_tests:
> > +                self.logger.info('TEST-SKIP | %s | filtered by type (%s)' % (
> > +                    os.path.basename(test),
> > +                    self.type))
> 
> So, the code starting on line 809 prints out the disabled tests; the code
> here prints out tests that were filtered out by --type args.  But, I don't
> see the equivalent anywhere of the original code which prints out tests that
> were disabled due to device type (e.g., skipped due to being a b2g desktop
> build).  Am I missing something?

These skip-if tests are essentially conditionally disabled, so they appear in the former list. See https://github.com/mozilla/mozbase/blob/6baa74e8b71374a2279a3b478617449adcd38423/manifestdestiny/manifestparser/manifestparser.py#L1065-1069

Comment 6

[Jonathan Griffin (:jgriffin)]

Comment on attachment 8365900 [details] [diff] [review]
List all skipped/disabled tests. v1.1

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

Ah, thanks for the explanation.  :)

Comment 7

[Dave Hunt [:davehunt] [he/him] ⌚BST]

This patch has an issue where the 'disabled' tests are still run. I'm working on a new version, which will also hopefully incorporate the additional skipped tests in the HTML/XML reports.

Comment 8

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: List all skipped/disabled tests. v2.0

Fixes the issues from the previous patch, and includes all skipped tests in the HTML/XML reports.

Comment 9

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: results.html

Example HTML report with the various skip types.

Comment 10

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: results.xml

Example XML report with the various skip types.

Comment 11

[Jonathan Griffin (:jgriffin)]

Comment on attachment 8366594 [details] [diff] [review]
List all skipped/disabled tests. v2.0

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

very nice!

Comment 12

[Jonathan Griffin (:jgriffin)]

I'll land this for you later today if the trees re-open.

Comment 13

[Jonathan Griffin (:jgriffin)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/23a89fe652c1

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/23a89fe652c1