[webkit-reviews] review denied: [Bug 31625] [Qt] It should be easier to run all Qt's autotests. : [Attachment 48899] script v2

[bugzilla-daemon at webkit.org]

Tor Arne Vestbø <vestbo at webkit.org> has denied Jędrzej Nowacki
<jedrzej.nowacki at nokia.com>'s request 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 48899: script v2
https://bugs.webkit.org/attachment.cgi?id=48899&action=review

------- Additional Comments from Tor Arne Vestbø <vestbo at webkit.org>
Squash into one script in WebKitTools/Scripts please.

Some other comments:


> +log = logging.getLogger("qttestmodule.Main")

This can then be changed to one log

> +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?

Why result? Wouldn't a better name be test?

> +    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

You should also use the "Can't open file: %s. Skipping" % (e.filename) syntax
to get rid of all those strs.

> +class TestSuiteResult(object):

Why the explicit inherit of object? 

> +    def set_test_file_name(self, fileName):

Should this be file_name?

> +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?

> +	     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.

> +	   debug("Found: " + str(len(tests_executables)))

See comment about "foo %s" % string syntax

> +    def find_tests_paths(self, path):
> +	   """Find all tests files (executables) inside a given path"""
> +	   result = []

results, it's an array, plural

> +    def run_tests(self, files):
> +	   """Run all tests given by path to executable files (iterable)"""
> +	   workers = self._Pool(processes = self._options.parallel_level)
> +	   #to each file add options
> +	   debug("Using %s s the workers pool", repr(workers))
> +	   package = map(lambda w: [w, self._options.tests_options, ], files)
> +	   debug("Package for worker: %s", repr(package))

%, probably lots of places

> +	   """Show result to public - result's shortcut should be written to
stdout"""

English nitpick, it's --- results'--- :)


That's it for now.

In general I'm wondering if the way output is processed is ideal. Looks like we
don't pick up stderr?

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?

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"?