Bug 31625 – [Qt] It should be easier to run all Qt's autotests.

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Jędrzej Nowacki]

Qt webkit port have own autotests, based on QTestLib framework. Purpose of the autotests is to test Qtish API, but in the same time it tests different features of WebKit. I believe it is valuable portion of tests: it is 7 suite case (about 140 test cases). Right now the tests are not executed automatically.

I propose to integrate Qt's autotests into layout tests, so they will be executed more frequently and results will be more accessible (visible on a web page).

[Jędrzej Nowacki]

I'm going to create solution in two steps:
1. Create layer that permits to call binary files from layout tests.
2. Create layout tests for the Qt API tests

[Jędrzej Nowacki]

Created attachment 43428 [details]
part1 - integration layer v1

The first part of patch

[Jędrzej Nowacki]

Created attachment 43429 [details]
part2 - layout tests v1

2nd part of patch

[Jędrzej Nowacki]

Should we disable somehow the tests on other ports than the Qt or it will happen automatically (tests are in platform/qt directory)?

[Jędrzej Nowacki]

After discussion on qtwebkit IRC channel, we decided that it would be better to create another step in build bot. The step should run all Qt's autotests. A broken step (test suite) will be more visible than just one broken layout test.

[Antonio Gomes]

(In reply to comment #5)
> After discussion on qtwebkit IRC channel, we decided that it would be better to
> create another step in build bot. The step should run all Qt's autotests. A
> broken step (test suite) will be more visible than just one broken layout test.

Jędrzej, WONTFIX then ?

[Jędrzej Nowacki]

No, with rgabor we tried to fix it, but it is not high priority. I've changed importance.

[Jędrzej Nowacki]

Created attachment 48758 [details]
Qt autotest launcher

[Tor Arne Vestbø]

Comment on attachment 48758 [details]
Qt autotest launcher

This should live in webkitpy, possibly in its own module/directory.

Please use pep8 for style:

http://www.python.org/dev/peps/pep-0008/

[Tor Arne Vestbø]

Also, there seems to be quite a few redundant/too verbose comments, you don't need to go all crazy with the comments :)

[Jędrzej Nowacki]

Created attachment 48899 [details]
script v2

[Tor Arne Vestbø]

Comment on attachment 48899 [details]
script v2

Now that i think about it, why does this need to be a module?

[Tor Arne Vestbø]

Comment on attachment 48899 [details]
script v2

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

[Jędrzej Nowacki]

Created attachment 49014 [details]
script v3

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

[Andras Becsi]

What's the status of this bug? Is there a chance that this could be resolved soon?

[Jędrzej Nowacki]

(In reply to comment #15)
> What's the status of this bug? Is there a chance that this could be resolved soon?
I haven't time to finsh the script after last review. It is not included here, but main issue to resolve was that there is no way to run it with sync stderr and stdout mode in one proccess.

I'll look at this in next week.

[Jędrzej Nowacki]

Created attachment 59646 [details]
script v4

Changes:
  - renamed to run-qtwebkit-tests
  - few small fixes in html generation
  - developer (synchronous and non parallel) mode
  - PEP8 style checked.

[Andras Becsi]

(In reply to comment #17)
> Created an attachment (id=59646) [details]
> script v4
> 
> Changes:
>   - renamed to run-qtwebkit-tests
>   - few small fixes in html generation
>   - developer (synchronous and non parallel) mode
>   - PEP8 style checked.

Thanks Jędrzej, looks good at first sight, we'll test it on our buildbot.

[Jędrzej Nowacki]

(In reply to comment #18)
> Thanks Jędrzej, looks good at first sight, we'll test it on our buildbot.
Great :-) I'm looking forward to your feedback.

[Gabor Rapcsanyi]

(In reply to comment #19)
> (In reply to comment #18)
> > Thanks Jędrzej, looks good at first sight, we'll test it on our buildbot.
> Great :-) I'm looking forward to your feedback.

It is working well. I have put it into our bots on http://sed.hu/webkit/qtbuildbot/waterfall

[Gabor Rapcsanyi]

(In reply to comment #20)
> (In reply to comment #19)
> > (In reply to comment #18)
> > > Thanks Jędrzej, looks good at first sight, we'll test it on our buildbot.
> > Great :-) I'm looking forward to your feedback.
> 
> It is working well. I have put it into our bots on http://sed.hu/webkit/qtbuildbot/waterfall

Sorry, the working link is http://webkit.sed.hu/buildbot/waterfall(In reply to comment #20)
> (In reply to comment #19)
> > (In reply to comment #18)
> > > Thanks Jędrzej, looks good at first sight, we'll test it on our buildbot.
> > Great :-) I'm looking forward to your feedback.
> 
> It is working well. I have put it into our bots on http://sed.hu/webkit/qtbuildbot/waterfall

Sorry, the working link is http://webkit.sed.hu/buildbot/waterfall

[Kenneth Rohde Christiansen]

Tor Arne, can you have a look at this again?

[Csaba Osztrogonác]

Comment on attachment 59646 [details]
script v4

cq- -ed, because if fix for https://bugs.webkit.org/show_bug.cgi?id=40672
isn't landed, it would brake our bots:http://webkit.sed.hu/buildbot/waterfall

[Jędrzej Nowacki]

(In reply to comment #23)
> (From update of attachment 59646 [details])
> cq- -ed, because if fix for https://bugs.webkit.org/show_bug.cgi?id=40672
> isn't landed, it would brake our bots:http://webkit.sed.hu/buildbot/waterfall
I'm curious, does it mean that the script would be run automatically? Can't we land it and after fixing 40672 just enable it for the build bot?

[Csaba Osztrogonác]

(In reply to comment #24)
/waterfall
> I'm curious, does it mean that the script would be run automatically? Can't we land it and after fixing 40672 just enable it for the build bot?

Yes, your script runs automatically on our bots (a long time ago). Now it has a local hack not to hang because of qwebhistory related bug.

It is possible to land first, but before it we have to disable unittest on the bots and remove this script from local svn working copy not to conflict when bot would like to download the landed script. And after fix for https://bugs.webkit.org/show_bug.cgi?id=40672 landed, we can enable unit tests on the bot again. Is that you really want?

[Jędrzej Nowacki]

(In reply to comment #25)
Thanks for explenation. I'm ok with waiting.

[Csaba Osztrogonác]

(In reply to comment #17)
> Created an attachment (id=59646) [details]
> script v4
Our bots now build WebKit against Qt-4.6.3, so I landed 
it manually in http://trac.webkit.org/changeset/62940 .