Closed Bug 952530 Opened 6 years ago Closed 6 years ago

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

Categories

(Testing :: Marionette, defect)

defect
Not set

Tracking

(b2g-v1.3 fixed)

RESOLVED FIXED
mozilla29
Tracking Status
b2g-v1.3 --- fixed

People

(Reporter: davehunt, Assigned: davehunt)

Details

Attachments

(3 files, 2 obsolete files)

Currently we output the tests conditionally skipped in the manifest files. Ideally we would list all skipped/disabled tests along with the reason.
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.
Assignee: nobody → dave.hunt
Status: NEW → ASSIGNED
Attachment #8350673 - Flags: feedback?(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.
Attachment #8350673 - Flags: feedback?(jgriffin) → feedback+
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?
Attachment #8365900 - Flags: review?(jgriffin)
Attachment #8350673 - Attachment is obsolete: true
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?
Attachment #8365900 - Flags: review?(jgriffin) → review-
(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 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.  :)
Attachment #8365900 - Flags: review- → review+
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.
Fixes the issues from the previous patch, and includes all skipped tests in the HTML/XML reports.
Attachment #8365900 - Attachment is obsolete: true
Attachment #8366594 - Flags: review?(jgriffin)
Attached file results.html
Example HTML report with the various skip types.
Attached file results.xml
Example XML report with the various skip types.
Comment on attachment 8366594 [details] [diff] [review]
List all skipped/disabled tests. v2.0

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

very nice!
Attachment #8366594 - Flags: review?(jgriffin) → review+
I'll land this for you later today if the trees re-open.
https://hg.mozilla.org/mozilla-central/rev/23a89fe652c1
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.