Closed Bug 787176 Opened 7 years ago Closed 7 years ago

Add a Python wrapper script for running C++ unit tests.

Categories

(Testing :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla19

People

(Reporter: ted, Assigned: ted)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

I'd like to add some features to C++ unit tests, notably the ability to get stack traces out of them. However, this requires a little bit of harness to wrap them. This patch adds a pretty minimal runcppunittests.py that simply sets some environment variables and executes the C++ unit test. It uses the new mozcrash module I implemented in bug 787119 to check for crashes, but none of the unit tests actually support that yet. Our C++ tests are a big mess, and fixing that is a much bigger patch, but I'd like to get this in so I can at least fix a few tests to use Breakpad and get stacks.
Attachment #656986 - Flags: review?(jmaher)
Comment on attachment 656986 [details] [diff] [review]
Add a Python wrapper script for running C++ unit tests.

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

Might be good to have this mach-ready
Attachment #656986 - Flags: feedback?(gps)
Comment on attachment 656986 [details] [diff] [review]
Add a Python wrapper script for running C++ unit tests.

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

these are just minor nits.  I don't know if you care about making these TBPL aware errors, then you might want to get TEST-UNEXPECTED-FAIL or something like that in the stdout/stderr.

::: testing/runcppunittest.py
@@ +32,5 @@
> +        print >>sys.stderr, """Error: --xre-path is required"""
> +        sys.exit(1)
> +
> +    prog = os.path.abspath(args[0])
> +    options.xrePath = os.path.abspath(options.xrePath)

can we verify this exists?  os.path.exists(options.xrePath)

@@ +61,5 @@
> +        mozcrash.check_for_crashes(tempdir, options.symbolsPath)
> +        return proc.proc.returncode
> +
> +if __name__ == '__main__':
> +    main()

return main() or sys.exit(main())
Attachment #656986 - Flags: review?(jmaher) → review+
Comment on attachment 656986 [details] [diff] [review]
Add a Python wrapper script for running C++ unit tests.

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

I like the idea, but I don't think this can land in its current form because the return value is being ignored.

I would *really* like to see this modulized sooner rather than later.

Also, is there any chance we can throw this in the virtualenv and run it from there? I hate these one-off python scripts in the source directory. I want everything to seem like a nice happy library to facilitate easier consumption of the tools.

::: testing/runcppunittest.py
@@ +33,5 @@
> +        sys.exit(1)
> +
> +    prog = os.path.abspath(args[0])
> +    options.xrePath = os.path.abspath(options.xrePath)
> +    #TODO: stick this in a module?

Definitely. And, the method for running an individual test should be a standalone API (no dependence on class state, etc) so that we can easily throw multiprocessing.Pool at the problem to get easy parallelism. Never mind that some tests may not like being executed in parallel right now (we can tackle that problem later when it presents itself). I just want us to engineer for optimal efficiency from the start rather than having to hack it in down the road.

Having it as a module will also enable mach to consume this easier.

Refactoring as a module also has other implications:

* Pass in sys.argv
* Don't call sys.exit

@@ +54,5 @@
> +        proc.run()
> +        timeout = 60
> +        proc.processOutput(timeout=timeout)
> +        if proc.timedOut:
> +            #TODO: log a failure

That seems like a required TODO to turn the tree orange/red, no?

@@ +61,5 @@
> +        mozcrash.check_for_crashes(tempdir, options.symbolsPath)
> +        return proc.proc.returncode
> +
> +if __name__ == '__main__':
> +    main()

sys.exit(main())? Otherwise, no return code.
Attachment #656986 - Flags: feedback?(gps) → feedback+
While I agree with all of these things in spirit, realize that the current C++ unit tests obey none of them. There's no consistent return codes on failure, no handling of timeouts, no parallelization, etc.

(In reply to Gregory Szorc [:gps] from comment #4)
> Also, is there any chance we can throw this in the virtualenv and run it
> from there? I hate these one-off python scripts in the source directory. I
> want everything to seem like a nice happy library to facilitate easier
> consumption of the tools.

I don't understand what this even means. It's a script, so why would we run it from the virtualenv? I'm happy to make it be importable as a module (it's close, just needs the sys.argv change, pretty much), but I don't get this.
Blocks: 787458
Depends on: 787119
Okay, I reworked this slightly so that the top-level __name__=='__main__' block handles the argument parsing and sys.exits with an error code on failures. It calls into a run_tests function, which uses a run_one_test function, all of which propogate errors correctly. As a bonus, I made it so we can pass all the tests in to a single invocation of the script and have it run them in series. It'd be straightforward to run them in parallel with this, but I didn't attempt that.

I found a test in xpcom/tests that needs MOZILLA_FIVE_HOME set, so I added that.
Attachment #657413 - Flags: review?(jmaher)
Attachment #657413 - Flags: feedback?(gps)
I provisionally landed this on alder to help debug some test crashes there:
http://hg.mozilla.org/projects/alder/rev/c3ebc4caa44c
Comment on attachment 657413 [details] [diff] [review]
Add a Python wrapper script for running C++ unit tests.

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

just some small details/nits.  otherwise this looks good.

::: testing/runcppunittests.py
@@ +4,5 @@
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +import sys, optparse, os, tempfile, shutil
> +import mozprocess, mozinfo, mozlog, mozcrash

where are these located at?  I thought we needed these in a virtual env or at the very least in the current path?

@@ +47,5 @@
> +            return False
> +        result = proc.proc.returncode == 0
> +        if not result:
> +            log.testFail("%s | test failed with return code %d",
> +                         basename, proc.proc.returncode)

we are not returning False here like we do in the failure conditions above.

@@ +48,5 @@
> +        result = proc.proc.returncode == 0
> +        if not result:
> +            log.testFail("%s | test failed with return code %d",
> +                         basename, proc.proc.returncode)
> +        return result

we are returning result, the comment above says True on success.  Can we be more specific about what we are returning.

@@ +83,5 @@
> +    #TODO: switch this to just abort once all C++ unit tests have
> +    # been fixed to enable crash reporting
> +    env["XPCOM_DEBUG_BREAK"] = "stack-and-abort"
> +    result = True
> +    for prog in progs:

I don't see progs defined in this function, should this be 'for prog in programs' ?
Attachment #657413 - Flags: review?(jmaher) → review+
(In reply to Joel Maher (:jmaher) from comment #8)
> > +import sys, optparse, os, tempfile, shutil
> > +import mozprocess, mozinfo, mozlog, mozcrash
> 
> where are these located at?  I thought we needed these in a virtual env or
> at the very least in the current path?

They're being used from the virtualenv. This script only needs to be run from an objdir, so that works just fine.

> > +        result = proc.proc.returncode == 0
> > +        if not result:
> > +            log.testFail("%s | test failed with return code %d",
> > +                         basename, proc.proc.returncode)
> 
> we are not returning False here like we do in the failure conditions above.

Right, because we return result immediately below this. Seemed redundant.

> > +        return result
> 
> we are returning result, the comment above says True on success.  Can we be
> more specific about what we are returning.

If you look, result is:
> result = proc.proc.returncode == 0

So it's always a boolean. If that's confusing I can tweak that.

> > +    for prog in progs:
> 
> I don't see progs defined in this function, should this be 'for prog in
> programs' ?

I have no idea how this code works. I get progs is being set as a global down in my __main__ block? crazy. Thanks!
Comment on attachment 657413 [details] [diff] [review]
Add a Python wrapper script for running C++ unit tests.

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

I'm not feeling well today. I trust jmaher's review abilities. Don't let me hold this up.
Attachment #657413 - Flags: feedback?(gps)
bug 788842 updated mozbase in m-c with the new mozcrash module.
Depends on: 788842
Attachment #656986 - Attachment is obsolete: true
This is a rollup of the patch here + a few followup fixes I needed to make things work reliably. I landed this all on alder a while back, now I'm going to land it on m-c.

r?Waldo for the TestHarness.h changes (but you're welcome to comment on the Python if you have any interest.)

This doesn't provide much up-front benefit, but it's a prerequisite to fix bug 787458.
Attachment #665922 - Flags: review?(jwalden+bmo)
Attachment #657413 - Attachment is obsolete: true
Attachment #665922 - Flags: review?(jwalden+bmo) → review+
Pushed to m-i:
https://hg.mozilla.org/integration/mozilla-inbound/rev/73a42225bf22

Also a prerequisite patch to add mozcrash to the Python virtualenv (I landed some form of this on alder at some point):
https://hg.mozilla.org/integration/mozilla-inbound/rev/9dc4dbee5c4f
https://hg.mozilla.org/mozilla-central/rev/73a42225bf22
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Blocks: 673017
Blocks: 719120
You need to log in before you can comment on or make changes to this bug.