Summary: | Add support for loading each test several times in chromium Python test runner | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Søren Gjesse <sgjesse> | ||||||||||
Component: | Tools / Tests | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | abarth, commit-queue, dglazkov, dpranke, levin, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
Attachments: |
|
Description
Søren Gjesse
2010-10-25 07:10:03 PDT
Created attachment 71745 [details]
Proposed patch.
Attachment 71745 [details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKitTools/Scripts/webkitpy/layout_tests/port/chromium.py:390: trailing whitespace [pep8/W291] [5]
WebKitTools/Scripts/webkitpy/layout_tests/port/chromium.py:396: indentation is not a multiple of four [pep8/E111] [5]
WebKitTools/Scripts/webkitpy/layout_tests/port/chromium.py:398: indentation is not a multiple of four [pep8/E111] [5]
Total errors found: 3 in 3 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 71746 [details]
Fixed style issues
Dirk/David/Adam, could you take a look? What are these flags used for (or what are they going to be used for)? AFAIK, old-run-webkit-tests has a --repeat-each flag, but that's implemented by looping in the script, not in DRT, and doesn't have a --js-flags script. Does upstream DRT support these flags, or just Chromium DRT? How does --multiple-loads actually interact with the framework? Do we only dump output for the first load, or the last load, or something? As far as the actual coding of the patch goes ... 1) Is it actually possible to pass an option in that preserves the quotation marks, or will you need to actually enclose the option in quotes every time (i.e., can both branches of that test execute)? 2) I'm not wild about using --multiple-loads=0 to indicate "use the default value". It seems like --multiple-loads=0 should turn off the feature, and I'm not sure what the savings would be over just typing --multiple-loads=5 ? (In reply to comment #5) > What are these flags used for (or what are they going to be used for)? AFAIK, old-run-webkit-tests has a --repeat-each flag, but that's implemented by looping in the script, not in DRT, and doesn't have a --js-flags script. > The main purpose is the enhance testing as we have seen bugs related to V8 which could only be revealed by loading the tests several times. While testing this new option the bug http://code.google.com/p/chromium/issues/detail?id=60135 was discovered. > Does upstream DRT support these flags, or just Chromium DRT? > No this is a chromium DRT only option for now. > How does --multiple-loads actually interact with the framework? Do we only dump output for the first load, or the last load, or something? > The --multiple-loads and --js-files are passed unmodified to DRT. Currently DRT will just dump the output for the last load. See https://bugs.webkit.org/show_bug.cgi?id=48233. > As far as the actual coding of the patch goes ... > > 1) Is it actually possible to pass an option in that preserves the quotation marks, or will you need to actually enclose the option in quotes every time (i.e., can both branches of that test execute)? > Well, only if you really want to e.g. --js-flags="\"--xxx --yyy\"", so I will remove that check. > 2) I'm not wild about using --multiple-loads=0 to indicate "use the default value". It seems like --multiple-loads=0 should turn off the feature, and I'm not sure what the savings would be over just typing --multiple-loads=5 ? Good point. I was thinking of having different defaults for release and debug (e.g. 5 and 2), but I could not get optparse of type "int" to handle just --multiple-loads (without equals some number). I have discarded the idea of a default for now. Created attachment 71848 [details]
Addressed comments #5
Comment on attachment 71848 [details]
Addressed comments #5
Change now looks good to me. Unfortunately, I'm not a review :(
Comment on attachment 71848 [details] Addressed comments #5 View in context: https://bugs.webkit.org/attachment.cgi?id=71848&action=review > WebKitTools/ChangeLog:6 > + Should point to the bug. Comment on attachment 71848 [details] Addressed comments #5 View in context: https://bugs.webkit.org/attachment.cgi?id=71848&action=review I'm willing to rs=me this. As Pavel said, your ChangeLog should point to the bug. If you're using webkit-patch upload it will create a properly formated changelog for you. You'll need to post a new patch if you want to use the commit-queue. > WebKitTools/Scripts/webkitpy/layout_tests/port/chromium.py:394 > + > + Why the extra newline? Created attachment 72155 [details]
Addressed comments #9 and #10
Comment on attachment 72155 [details] Addressed comments #9 and #10 Clearing flags on attachment: 72155 Committed r70789: <http://trac.webkit.org/changeset/70789> All reviewed patches have been landed. Closing bug. |