Bug 48236 - Add support for loading each test several times in chromium Python test runner
Summary: Add support for loading each test several times in chromium Python test runner
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-10-25 07:10 PDT by Søren Gjesse
Modified: 2010-10-28 11:08 PDT (History)
6 users (show)

See Also:


Attachments
Proposed patch. (2.68 KB, patch)
2010-10-25 07:12 PDT, Søren Gjesse
no flags Details | Formatted Diff | Diff
Fixed style issues (2.68 KB, patch)
2010-10-25 07:37 PDT, Søren Gjesse
no flags Details | Formatted Diff | Diff
Addressed comments #5 (2.44 KB, patch)
2010-10-26 01:02 PDT, Søren Gjesse
eric: review+
eric: commit-queue-
Details | Formatted Diff | Diff
Addressed comments #9 and #10 (2.49 KB, patch)
2010-10-28 00:51 PDT, Søren Gjesse
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Søren Gjesse 2010-10-25 07:10:03 PDT
The Python test runner needs to support the DumpRenderTree flags --multiple-loads and --js-flags.
Comment 1 Søren Gjesse 2010-10-25 07:12:55 PDT
Created attachment 71745 [details]
Proposed patch.
Comment 2 WebKit Review Bot 2010-10-25 07:17:26 PDT
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.
Comment 3 Søren Gjesse 2010-10-25 07:37:42 PDT
Created attachment 71746 [details]
Fixed style issues
Comment 4 Dimitri Glazkov (Google) 2010-10-25 07:47:05 PDT
Dirk/David/Adam, could you take a look?
Comment 5 Dirk Pranke 2010-10-25 11:51:24 PDT
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 ?
Comment 6 Søren Gjesse 2010-10-26 00:54:15 PDT
(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.
Comment 7 Søren Gjesse 2010-10-26 01:02:56 PDT
Created attachment 71848 [details]
Addressed comments #5
Comment 8 Dirk Pranke 2010-10-26 12:21:38 PDT
Comment on attachment 71848 [details]
Addressed comments #5

Change now looks good to me. Unfortunately, I'm not a review :(
Comment 9 Pavel Feldman 2010-10-27 06:11:17 PDT
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 10 Eric Seidel (no email) 2010-10-27 16:45:28 PDT
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?
Comment 11 Søren Gjesse 2010-10-28 00:51:57 PDT
Created attachment 72155 [details]
Addressed comments #9 and #10
Comment 12 WebKit Commit Bot 2010-10-28 11:08:30 PDT
Comment on attachment 72155 [details]
Addressed comments #9 and #10

Clearing flags on attachment: 72155

Committed r70789: <http://trac.webkit.org/changeset/70789>
Comment 13 WebKit Commit Bot 2010-10-28 11:08:38 PDT
All reviewed patches have been landed.  Closing bug.