On 2012/09/28 17:38:52, Jakob wrote:
I personally don't need this, but if it makes your lives easier, that's
fine,
and I have no objections to adding this functionality.
However: tools/test.py is deprecated, along with
tools/test-wrapper-gypbuild.py.
Please integrate this into the new test driver instead
(tools/run-tests.py +
tools/testrunner/local/*.py). You should find that it is much easier to
work
with than the old script ;-)
In particular, look at tools/testrunner/local/progress.py (where you can
add
the
new indicator) and .../execution.py (to understand how/when its methods
are
called).
The comments below assume that you did this, and apply in addition.
https://chromiumcodereview.appspot.com/11000052/diff/1/tools/test.py
File tools/test.py (right):
https://chromiumcodereview.appspot.com/11000052/diff/1/tools/test.py#newcode49
tools/test.py:49: XMLOUT = None
No global variables. Please.
https://chromiumcodereview.appspot.com/11000052/diff/1/tools/test.py#newcode72
tools/test.py:72: if XMLOUT:
This looks weird. The UnitTestProgressIndicator is-a ProgressIndicator
according
to its class definition (which is good!). The additional has-a relation
here
is
counter-intuitive. Why doesn't the UnitTestProgressIndicator simply
replace
any
other progress indicator in use? If you want to print progress
information to
stdout at the same time as dumping the XML file, you can always derive
from
one
of the printing progress indicators (and call the superclass methods when
you
override them).
This comment also applies to the next few edits below.
https://chromiumcodereview.appspot.com/11000052/diff/1/tools/test.py#newcode320
tools/test.py:320:
nit: two newlines between top-level definitions
https://chromiumcodereview.appspot.com/11000052/diff/1/tools/unittest_output.py
File tools/unittest_output.py (right):
https://chromiumcodereview.appspot.com/11000052/diff/1/tools/unittest_output.py#newcode1
tools/unittest_output.py:1: # Copyright 2008 the V8 project authors. All
rights
reserved.
nit: 2012
https://chromiumcodereview.appspot.com/11000052/diff/1/tools/unittest_output.py#newcode33
tools/unittest_output.py:33: import os
nit: please sort imports alphabetically
https://chromiumcodereview.appspot.com/11000052/diff/1/tools/unittest_output.py#newcode34
tools/unittest_output.py:34:
nit: two newlines between top-level definitions
https://chromiumcodereview.appspot.com/11000052/diff/1/tools/unittest_output.py#newcode35
tools/unittest_output.py:35: def getProperTestName(name):
Why do you need to extract the test's name from its command line?
https://chromiumcodereview.appspot.com/11000052/diff/1/tools/unittest_output.py#newcode42
tools/unittest_output.py:42:
nit: two newlines between top-level definitions
https://chromiumcodereview.appspot.com/11000052/diff/1/tools/unittest_output.py#newcode48
tools/unittest_output.py:48: self.starttime = time.time()
No reason to measure execution time here. It is measured anyway and you
can
fetch it from the testcase object. Which means you don't need any custom
behavior when a test starts, and can put everything into the progress
indicator's HasRun() method.
https://chromiumcodereview.appspot.com/11000052/diff/1/tools/unittest_output.py#newcode61
tools/unittest_output.py:61: class UnitTestLocal(threading.local):
In the new test driver, the progress indicators are no longer threaded,
which
should make this class unnecessary (and some stuff below simpler).
Hi Jakob,
Thanks for the detailed review, it was very helpful for me!
I've implemented the JUnit test output in the run-tests.py and uploaded to a
separate CL:
https://codereview.chromium.org/13813003
Please take a look.
https://codereview.chromium.org/11000052/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.
