Bug 48236 – Add support for loading each test several times in chromium Python test runner

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

[Søren Gjesse]

The Python test runner needs to support the DumpRenderTree flags --multiple-loads and --js-flags.

[Søren Gjesse]

Created attachment 71745 [details]
Proposed patch.

[WebKit Review Bot]

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.

[Søren Gjesse]

Created attachment 71746 [details]
Fixed style issues

[Dimitri Glazkov (Google)]

Dirk/David/Adam, could you take a look?

[Dirk Pranke]

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 ?

[Søren Gjesse]

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

[Søren Gjesse]

Created attachment 71848 [details]
Addressed comments #5

[Dirk Pranke]

Comment on attachment 71848 [details]
Addressed comments #5

Change now looks good to me. Unfortunately, I'm not a review :(

[Pavel Feldman]

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.

[Eric Seidel (no email)]

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?

[Søren Gjesse]

Created attachment 72155 [details]
Addressed comments #9 and #10

[WebKit Commit Bot]

Comment on attachment 72155 [details]
Addressed comments #9 and #10

Clearing flags on attachment: 72155

Committed r70789: <http://trac.webkit.org/changeset/70789>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.