Summary: | [Qt] It should be easier to run all Qt's autotests. | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jędrzej Nowacki <jedrzej.nowacki> | ||||||||||||||
Component: | Tools / Tests | Assignee: | Jędrzej Nowacki <jedrzej.nowacki> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Minor | CC: | abecsi, hausmann, jedrzej.nowacki, kenneth, ossy, rgabor, tonikitoo, vestbo | ||||||||||||||
Priority: | P3 | Keywords: | Qt | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | PC | ||||||||||||||||
OS: | Linux | ||||||||||||||||
Bug Depends on: | 40672 | ||||||||||||||||
Bug Blocks: | |||||||||||||||||
Attachments: |
|
Description
Jędrzej Nowacki
2009-11-18 04:51:34 PST
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 Created attachment 43428 [details]
part1 - integration layer v1
The first part of patch
Created attachment 43429 [details]
part2 - layout tests v1
2nd part of patch
Should we disable somehow the tests on other ports than the Qt or it will happen automatically (tests are in platform/qt directory)? 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. (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 ? No, with rgabor we tried to fix it, but it is not high priority. I've changed importance. Created attachment 48758 [details]
Qt autotest launcher
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/ Also, there seems to be quite a few redundant/too verbose comments, you don't need to go all crazy with the comments :) Created attachment 48899 [details]
script v2
Comment on attachment 48899 [details]
script v2
Now that i think about it, why does this need to be a module?
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"? 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? What's the status of this bug? Is there a chance that this could be resolved soon? (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. 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.
(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. (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. (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 (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 Tor Arne, can you have a look at this again? 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 (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? (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? (In reply to comment #25) Thanks for explenation. I'm ok with waiting. (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 . |