[webkit-reviews] review requested: [Bug 31625] [Qt] It should be easier to run all Qt's autotests. : [Attachment 49014] script v3

[bugzilla-daemon at webkit.org]

Jędrzej Nowacki <jedrzej.nowacki at nokia.com> has asked  for review:
Bug 31625: [Qt] It should be easier to run all Qt's autotests.
https://bugs.webkit.org/show_bug.cgi?id=31625

Attachment 49014: script v3
https://bugs.webkit.org/attachment.cgi?id=49014&action=review

------- Additional Comments from Jędrzej Nowacki <jedrzej.nowacki at nokia.com>
(In reply to comment #13)
> (From update of attachment 48899 [details])
> Squash into one script in WebKitTools/Scripts please.
Done.

> > +log = logging.getLogger("qttestmodule.Main")
> This can then be changed to one log
Logging style was completely changed, granularity was changed from module to
class level.

> > +def run_test(args):
> > +	 """Run one given test"""
> > +	 result, options = args
>
> Why a single argument that you then split up instead of two? Is it due to the

> workers?
Yes, arguments have to be packed because of Pool::map(func, iterable[,
chunksize]).

> Why result? Wouldn't a better name be test?
Right, changed to test_suite.

> > +	 try:
> > +	     info("Running... " + result.test_file_name())
> > +	     tst = Popen(result.test_file_name() + options, stdout = PIPE,
stderr = None, shell = True)
> > +	 except OSError, e:
> > +	     exception("Can't open a file: " + str(e.filename) + ".
Skipping...")
>
> Strike the "a" in the string
Done.

> You should also use the "Can't open file: %s. Skipping" % (e.filename) syntax

> to get rid of all those strs.
No str() left :-), but why? Btw. syntax you proposed is slower and confusing
(it looks like printf aghr :-)).

> > +class TestSuiteResult(object):
> Why the explicit inherit of object?
There is a difference between class Foo and class Foo(object). In general you
should always inherit the object (or another type) to use new-style classes.
The issue should be removed in python 3.0, but I'm not sure.

> > +	 def set_test_file_name(self, fileName):
>
> Should this be file_name?
It should, fixed.

> > +class Main(object):
> > +	 """Main script. All work is done in run()"""
>
> Do we really need to split this into a class with init and then run? How
about
> just a main function with helper functions like run_tests etc?
I'm using class attribute to pass options. I think that it is possible to write
it using only one function definition (run_test), but why?

> > +	       class Pool(object):
> > +		   """Hack, avoid problems with multiprocessing module, this
class is single thread replacement
> > +		   for multiprocessing.Pool class. "Replacement" hehehe """
>
> You can strike the last part of that comment.
Done :D.

> > +	 def find_tests_paths(self, path):
> > +	     """Find all tests files (executables) inside a given path"""
> > +	     result = []
> results, it's an array, plural
Result of the method is only one... Changed to 'executables'.

> > +	     """Show result to public - result's shortcut should be written to
stdout"""
>
> English nitpick, it's --- results'--- :)
Cool, changed.

> In general I'm wondering if the way output is processed is ideal. Looks like
we
> don't pick up stderr?
True, I don't touch stderr. We can try to improve it in future, but parsing
will be complex.

>
http://www.sed.hu/webkit/qtbuildbot/builders/x86-32%20Linux%20Qt%20Debug/builds
/232/steps/qt-unit-test/logs/stdio
>
> It seems some of the output is spit out when running the tests, and then we
> flush the output later?
Right. At the beginning infos, warning, errors and stderr are spit out, then
stdout is flushed.

> What happens if I run this on the command line myself, will I get continuous
> output as if I had a for-loop on the shell to run each test in succession, or

> will I have to wait until all the tests are run before the "results are
> published"?
Results are published at the end of processing. This is the price for running
all tests in parallel.

What do you think about this version?