Closed Bug 729098 Opened 8 years ago Closed 6 years ago

Add an easy way to rerun xpcshell tests that failed the last time

Categories

(Testing :: XPCShell Harness, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla26

People

(Reporter: Irving, Assigned: chmanchester)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [mozbase])

Attachments

(3 files, 7 obsolete files)

8.09 KB, patch
khuey
: review+
Details | Diff | Splinter Review
26.50 KB, patch
ted
: review+
Details | Diff | Splinter Review
10.80 KB, patch
gps
: review+
Details | Diff | Splinter Review
+++ This bug was initially created as a clone of Bug #725112 +++

One of the pain points Irving mentioned in a discussion about how difficult it is to write code for Mozilla is the lack of an easy way to run whichever tests failed the last time tests were run. We should add a make target that does just that. It might make sense to have separate targets per-test harness, or at least separate log files.
Bug 725478 landed. So, we now have the ability to produce (easily) machine readable xpcshell output.

For this bug, I'd like to modify the build system to produce xUnit XML files by default. Then, we can add some plumbing to parse these files to run the failed tests. Finally, we hook up the plumbing with a new Makefile target.

I just need to know where to put the generated xUnit files...
Here is part 1 to write out the results file from the global xpcshell-tests target. It is pretty trivial.
Assignee: nobody → gps
Status: NEW → ASSIGNED
Attachment #601167 - Flags: review?(bmo)
This adds xUnit XML support to the per-directory xpcshell-tests target.

I'm pretty sure $(relativesrcdir) has paths normalized to / on all platforms, but I haven't tested on Windows.

Also, extracting paths from the produced XML files could be troublesome. We may require more integration with runxpcshelltests.py.
Attachment #601169 - Flags: review?(bmo)
Comment on attachment 601169 [details] [diff] [review]
Part 2: Write xUnit XML for per-directory xpcshell-tests target

Silly Mercurial didn't pick up the same change in js/src/config/rules.mk. Rest assured it is made in my local repository.
Comment on attachment 601167 [details] [diff] [review]
Part 1: Write xUnit XML file for global xpcshell-tests target

Really sorry, I'm not a peer, transferring to khuey.
Attachment #601167 - Flags: review?(bmo) → review?(khuey)
Attachment #601169 - Flags: review?(bmo) → review?(khuey)
I combined the patches and modified runxpcshelltests.py so the test's "class" is munged from the relative directory the test is contained within.

Things seem to work well! Jenkins has no problem parsing the XML file, so I imagine other tools will fare the same. See http://jenkins.gregoryszorc.com:9000/job/mozilla-central-linux-x64-optimized-llvm-tip-tests-xpcshell/lastCompletedBuild/testReport/ for an example of what Jenkins displays.

I can think of the following potential issues:

1) Currently the XML doesn't include tests skipped as a result of CTRL+C. This is consistent with the console output. However, for executing "failed" tests (future part 2), we may reconsider this approach to expand "failed" to include tests that didn't execute.

2) If/when we add xUnit generation to other test runners, we may have to differentiate the different results somehow (possibly by munging the "class" name further. I figure we'll cross that bridge if/when we get to it.

Try build at http://tbpl.mozilla.org/?tree=Try&rev=4521732b43e7
Attachment #601167 - Attachment is obsolete: true
Attachment #601169 - Attachment is obsolete: true
Attachment #602808 - Flags: review?(khuey)
Attachment #601167 - Flags: review?(khuey)
Attachment #601169 - Flags: review?(khuey)
Gah. Forgot to refresh js's rules.mk. Hopefully non-busted Try at http://tbpl.mozilla.org/?tree=Try&rev=3e210bb1ab28
Try run for 4521732b43e7 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=4521732b43e7
Results (out of 8 total builds):
    exception: 3
    failure: 5
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/gszorc@mozilla.com-4521732b43e7
Added missing js/src/config/rules.mk changes.
Fixed unit tests.
Attachment #602808 - Attachment is obsolete: true
Attachment #602817 - Flags: review?(khuey)
Attachment #602808 - Flags: review?(khuey)
This patch adds the Makefile plumbing to re-run failed tests.

I had to implement a parser for the xUnit XML. I figured this was as good a time as any to move the XML functionality to another, more generic module [so other test harnesses can easily leverage it]. I'm not sure if I put it in the right place. But, I don't know where else would be better: the Python in the tree is a mess.

Try at http://tbpl.mozilla.org/?tree=Try&rev=0697cb377aea
Attachment #602818 - Flags: review?(khuey)
Old patch had broken the test harness when executed without --tests-root-dir. This version fixes it.

Try at http://tbpl.mozilla.org/?tree=Try&rev=d0b5afb300d1

Someone will also have to explain to me how the build farm launches xpcshell tests. It is apparently not running the normal make target from testing/testsuite-targets.mk because I don't see the xunit args being passed to runxpcshelltests.py.
Attachment #602817 - Attachment is obsolete: true
Attachment #602819 - Flags: review?(khuey)
Attachment #602817 - Flags: review?(khuey)
Try run for 3e210bb1ab28 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=3e210bb1ab28
Results (out of 15 total builds):
    success: 2
    warnings: 13
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/gszorc@mozilla.com-3e210bb1ab28
Try run for 0697cb377aea is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=0697cb377aea
Results (out of 15 total builds):
    success: 8
    warnings: 7
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/gszorc@mozilla.com-0697cb377aea
Try run for d0b5afb300d1 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=d0b5afb300d1
Results (out of 15 total builds):
    success: 14
    warnings: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/gszorc@mozilla.com-d0b5afb300d1
Comment on attachment 602818 [details] [diff] [review]
Part 2: Add xpcshell-tests-rerun-failed make target

Not a peer.
Attachment #602818 - Flags: review?(khuey) → review?(ted.mielczarek)
Comment on attachment 602819 [details] [diff] [review]
Part 1: Write xUnit XML when executing xpcshell tests

Not really a peer of test stuff, but this is mostly build stuff and looks fine.
Attachment #602819 - Flags: review?(khuey) → review+
Btw I think you misnamed those two patches.
Part 1: https://hg.mozilla.org/integration/mozilla-inbound/rev/8ee446a837d5
Whiteboard: [do not resolve bug on inbound merge]
Blocks: 733960
Fix obvious DRY violation.
Attachment #602818 - Attachment is obsolete: true
Attachment #606302 - Flags: review?(ted.mielczarek)
Attachment #602818 - Flags: review?(ted.mielczarek)
Comment on attachment 606302 [details] [diff] [review]
Part 2: Add xpcshell-tests-rerun-failed make target

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

r- for a few things, the only big thing is that I'd like Mochitest and xpcshell to use the same (or very similar) arguments for this feature. Feel free to change Mochitest as well.

This could also use a unit test in selftest.py.

::: build/automationutils.py
@@ +589,5 @@
> +    name -- Name of the test suite. Many tools expect Java class dot notation
> +        e.g. dom.simple.foo. A directory with '/' converted to '.' is a good
> +        choice.
> +    fh -- File handle to write XML to.
> +    filename -- File name to write XML to.

It would be nice to condense these two arguments into one "output" argument, and just do the right thing depending on what's passed in, like "if hasattr(output, "write"): ... elif isinstance(output, basestring): ...".

@@ +596,5 @@
> +    if filename is None and fh is None:
> +        raise Exception("One of filename or fh must be defined.")
> +
> +    if name is None:
> +        name = "xpcshell"

This doesn't make sense in automationutils.py.

@@ +621,5 @@
> +            skipped += 1
> +        elif result["passed"]:
> +            passed += 1
> +        else:
> +            failed += 1

Does XUnit have the concept of "todo" tests? We support those in lots of our test suites.

@@ +662,5 @@
> +    testsuite.setAttribute("skip", str(skipped))
> +
> +    doc.writexml(fh, addindent="  ", newl="\n", encoding="utf-8")
> +
> +    return doc

Is there a useful reason to return the doc after writing it?

::: config/rules.mk
@@ +141,5 @@
>  testxpcsrcdir = $(topsrcdir)/testing/xpcshell
>  
>  # Execute all tests in the $(XPCSHELL_TESTS) directories.
>  # See also testsuite-targets.mk 'xpcshell-tests' target for global execution.
> +xpcshell_command = $(PYTHON) \

Can you call this "run_xpcshell" for consistency with the Mochitest one?

@@ +145,5 @@
> +xpcshell_command = $(PYTHON) \
> +  -u $(topsrcdir)/config/pythonpath.py \
> +  -I$(topsrcdir)/build \
> +  $(testxpcsrcdir)/runxpcshelltests.py \
> +  --symbols-path=$(DIST)/drashreporter-symbols \

Typo: "crashreporter".

@@ +158,5 @@
>  xpcshell-tests:
> +	$(xpcshell_command)
> +
> +xpcshell-tests-rerun-failures:
> +	$(xpcshell_command) --failed-only

It would be nice to make the command-line bits to invoke this functionality the same between xpcshell and Mochitest, even if the underlying functionality is not the same. Can you figure something out there? (Even if you want to tweak Mochitest to meet in the middle, that's fine.)

::: testing/testsuite-targets.mk
@@ +245,5 @@
> +  --xunit-file=$(call core_abspath,_tests/xpcshell/results.xml) \
> +  --xunit-suite-name=xpcshell \
> +  $(SYMBOLS_PATH) \
> +  $(TEST_PATH_ARG) $(EXTRA_TEST_ARGS) \
> +  $(LIBXUL_DIST)/bin/xpcshell

We should probably figure out how to sync this up with the stuff in testsuite-targets. Can you file a followup on that?

::: testing/xpcshell/runxpcshelltests.py
@@ +688,5 @@
>  INFO | Failed: %d
>  INFO | Todo: %d""" % (self.passCount, self.failCount, self.todoCount))
>  
> +    if self.failCount > 0:
> +        self.log.info("To re-run failed tests, run 'make xpcshell-tests-rerun-failures'")

This would probably fit better conceptually in the Makefile goop in testsuite-targets.mk, but I can deal with it either way. It'll just be a little weird to see it output in tinderbox logs. :)
Attachment #606302 - Flags: review?(ted.mielczarek) → review-
(In reply to Ted Mielczarek [:ted] from comment #21)
> Comment on attachment 606302 [details] [diff] [review]
> Part 2: Add xpcshell-tests-rerun-failed make target
> 
> Review of attachment 606302 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r- for a few things, the only big thing is that I'd like Mochitest and
> xpcshell to use the same (or very similar) arguments for this feature. Feel
> free to change Mochitest as well.

I'm not sure that is possible. See comment below.

> This could also use a unit test in selftest.py.

Will add one!

> 
> @@ +621,5 @@
> > +            skipped += 1
> > +        elif result["passed"]:
> > +            passed += 1
> > +        else:
> > +            failed += 1
> 
> Does XUnit have the concept of "todo" tests? We support those in lots of our
> test suites.

XUnit supports "skipped" tests. We support this in runxpcshelltests:

      if 'disabled' in test:
        self.log.info("TEST-INFO | skipping %s | %s" %
                      (name, test['disabled']))

        xunitResult["skipped"] = True
        xunitResults.append(xunitResult)
        continue

> 
> @@ +158,5 @@
> >  xpcshell-tests:
> > +	$(xpcshell_command)
> > +
> > +xpcshell-tests-rerun-failures:
> > +	$(xpcshell_command) --failed-only
> 
> It would be nice to make the command-line bits to invoke this functionality
> the same between xpcshell and Mochitest, even if the underlying
> functionality is not the same. Can you figure something out there? (Even if
> you want to tweak Mochitest to meet in the middle, that's fine.)

As you said, the underlying functionality is not the same. I suggested in bug 725112 that mochitests adopt XUnit XML files for this feature because it has usage beyond this use case. That request was not heeded. I would like to change the mochitest test runner to produce this style of files. This patch will enable that because the XUnit XML file manipulation logic is being moved to a shared Python module. Perhaps your request here is better left as a follow-up bug?

> ::: testing/testsuite-targets.mk
> @@ +245,5 @@
> > +  --xunit-file=$(call core_abspath,_tests/xpcshell/results.xml) \
> > +  --xunit-suite-name=xpcshell \
> > +  $(SYMBOLS_PATH) \
> > +  $(TEST_PATH_ARG) $(EXTRA_TEST_ARGS) \
> > +  $(LIBXUL_DIST)/bin/xpcshell
> 
> We should probably figure out how to sync this up with the stuff in
> testsuite-targets. Can you file a followup on that?

I'm pretty sure I saw this mentioned in a joey-related makefile refactor bug. I can't find the specific comment though. Do you still need a follow-up?

> 
> ::: testing/xpcshell/runxpcshelltests.py
> @@ +688,5 @@
> >  INFO | Failed: %d
> >  INFO | Todo: %d""" % (self.passCount, self.failCount, self.todoCount))
> >  
> > +    if self.failCount > 0:
> > +        self.log.info("To re-run failed tests, run 'make xpcshell-tests-rerun-failures'")
> 
> This would probably fit better conceptually in the Makefile goop in
> testsuite-targets.mk, but I can deal with it either way. It'll just be a
> little weird to see it output in tinderbox logs. :)

I agree. Although, it makes the Makefile logic a bit more complicated. No big deal, though. I'll move it.
I didn't move the logic to display "re-run last failed test" to the Makefiles because I didn't feel like writing Makefile today. Since you were OK with it in the last review, I assume you will be OK with it :)

Try at http://tbpl.mozilla.org/?tree=Try&rev=804a7b4e4487
Attachment #606302 - Attachment is obsolete: true
Attachment #616238 - Flags: review?(ted.mielczarek)
Try run for 804a7b4e4487 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=804a7b4e4487
Results (out of 16 total builds):
    success: 13
    warnings: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/gszorc@mozilla.com-804a7b4e4487
Attachment 616238 [details] [diff] did not pass tests on Windows :(
FYI, the patch in bug 732638 should make it trivial to move the "rerun" message to the Makefile section.
Depends on: 732638
Comment on attachment 616238 [details] [diff] [review]
Part 2: Add xpcshell-tests-rerun-failed make target, v2

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

::: build/automationutils.py
@@ +591,5 @@
> +        e.g. dom.simple.foo. A directory with '/' converted to '.' is a good
> +        choice.
> +    """
> +    if not isinstance(name, str):
> +        raise Exception("name argument must be a str.")

Seems a little strict, why not just run str() on it? (Do you think it will be easy to screw up the usage?)

@@ +652,5 @@
> +    testsuite.setAttribute("skip", str(skipped))
> +
> +    return doc
> +
> +def writeXunitResults( results, name, fh):

extraneous space after the open paren.

@@ +666,5 @@
> +    doc = createXunitDocument(results, name)
> +    doc.writexml(fh, addindent="  ", newl="\n", encoding="utf-8")
> +
> +def readXunitResults(filename):
> +    """Read an xUnit XML results file back into a data structure.

You have inconsistent formatting with your """ here vs. elsewhere.

@@ +668,5 @@
> +
> +def readXunitResults(filename):
> +    """Read an xUnit XML results file back into a data structure.
> +
> +    The filename argument can be either a filename or file handle object.

Seems odd to call it "filename" then. I'd say "file" but that shadows the global "file" constructor.
Attachment #616238 - Flags: review?(ted.mielczarek) → review+
Blocks: 752252
Depends on: 773394
I don't intend on landing this patch in its current form. Instead, I plan to integrate this functionality into mach (bug 751795). This is in the spirit of bug 769394.
Blocks: 769394
Depends on: mach
Summary: Add an easy make command to rerun xpcshell tests that failed the last time → Add an easy way to rerun xpcshell tests that failed the last time
Blocks: machfeatures
No longer depends on: mach
So xpcshell uses test manifests from ManifestDestiny: https://github.com/mozilla/mozbase/tree/master/manifestdestiny . Perhaps instead of the xunit format, we can/should use ManifestDestiny to output a manifest of failing tests?  This can be done in harness, I think.  I'm also more than happy to add upstream fixes to ManifestDestiny if any are needed and this road is taken
(In reply to Jeff Hammel [:jhammel] from comment #29)
> So xpcshell uses test manifests from ManifestDestiny:
> https://github.com/mozilla/mozbase/tree/master/manifestdestiny . Perhaps
> instead of the xunit format, we can/should use ManifestDestiny to output a
> manifest of failing tests?  This can be done in harness, I think.  I'm also
> more than happy to add upstream fixes to ManifestDestiny if any are needed
> and this road is taken

I really don't care what the format for caching the failed tests is, as long as it can be produced and consumed via an API that doesn't involve parsing logs. I initially went with xunit files because I had just implemented an xunit writer for xpcshell tests and that's as machine readable as anything.
Depends on: 795772
Returning to the pool. I can't even remember what the state of this bug is. We should probably abandon a make target in favor of a mach argument.
Assignee: gps → nobody
I'm going to readvocate what I advocated in comment 29: the failing files are output to a manifestparser.py manifest that can be rerun (either with automation -- i.e. mach -- or manually). 

Assuming there's no dissension, a blocking bug should be filed to make this a general pattern (as applicable) since we'll really want this for all manifestparser.py-backed frameworks. (I'll file if I can pop stack fast enough not to forget to.)
Whiteboard: [mozbase]
Blocks: 860005
The "write a test object to failure manifest" part of this is sort of a hack,
but until we have a facility for representing and modifying a set of test data
dynamically, it appears to do the right thing.

This has nominal compatibility with the way this is done in mochitests, but the
file formats used are different.
Attachment #787647 - Flags: feedback?(jhammel)
Attachment #787647 - Flags: feedback?(gps)
Comment on attachment 787647 [details] [diff] [review]
Add rerun-failures as an option to mach, add a failure manifest option to runxpcshelltests.py;

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

Why only f?. This is so close to r+!

::: testing/xpcshell/mach_commands.py
@@ +141,5 @@
> +        # failure set.
> +        failure_manifest_path = os.path.join(self.statedir, 'xpcshell.failures.ini')
> +        if os.path.exists(failure_manifest_path) and rerun_failures:
> +            args['manifest'] = failure_manifest_path
> +            args['failureManifest'] = None

One of the use cases of a failure manifest is to keep running only the failed tests until all tests pass. e.g. First you get 8 failures, then 6, then 4, etc. You want to keep running only the most recently failed tests on each invocation. This requires saving the new failure manifest after every run - something you don't do here.
Attachment #787647 - Flags: feedback?(gps) → feedback+
Comment on attachment 787647 [details] [diff] [review]
Add rerun-failures as an option to mach, add a failure manifest option to runxpcshelltests.py;

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

::: testing/xpcshell/mach_commands.py
@@ +147,5 @@
> +            os.remove(failure_manifest_path)
> +            args['failureManifest'] = failure_manifest_path
> +        else:
> +            args['failureManifest'] = failure_manifest_path
> +

I'd probably refactor the args['failureManifest'] out a bit (not combine the initial conditions of exists and rerun_failures) but this is fine.

@@ +183,5 @@
>          help='Continue running tests after a SIGINT is received.')
>      @CommandArgument('--shuffle', '-s', action='store_true',
>          help='Randomize the execution order of tests.')
> +    @CommandArgument('--rerun-failures', action='store_true',
> +        help='Reruns failures from last time.')

Shouldn't the failure manifest be specified too?

::: testing/xpcshell/runxpcshelltests.py
@@ +1010,5 @@
>                  }
> +
> +                if self.failureManifest:
> +                    with open(self.failureManifest, 'a') as f:
> +                        f.write('[%s]\n' % test['name'])

You might want test['path'] for absolute paths.  Not sure of the entirety of the intention here
Attachment #787647 - Flags: feedback?(jhammel) → feedback+
> @@ +183,5 @@
> >          help='Continue running tests after a SIGINT is received.')
> >      @CommandArgument('--shuffle', '-s', action='store_true',
> >          help='Randomize the execution order of tests.')
> > +    @CommandArgument('--rerun-failures', action='store_true',
> > +        help='Reruns failures from last time.')
> 
> Shouldn't the failure manifest be specified too?

I was thinking this would keep in line with the "just works" mentality behind mach commands, so that whoever uses this wont be managing their own failure manifests. Is there a compelling use case for being able to specify one's own manifests?

> 
> ::: testing/xpcshell/runxpcshelltests.py
> @@ +1010,5 @@
> >                  }
> > +
> > +                if self.failureManifest:
> > +                    with open(self.failureManifest, 'a') as f:
> > +                        f.write('[%s]\n' % test['name'])
> 
> You might want test['path'] for absolute paths.  Not sure of the entirety of
> the intention here

Yes, I think that will work better.
Attachment #787647 - Attachment is obsolete: true
Assignee: nobody → cmanchester
Comment on attachment 793534 [details] [diff] [review]
Add rerun-failures as an option to mach, add a failure manifest option to runxpcshelltests.py;

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

LGTM!
Attachment #793534 - Flags: review?(gps) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0b6aa8fab175
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.