Bug 61088 – run-api-tests should run one test per process

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

[Dmitry Lomov]

It currently runs all tests in the same process. This:
1) Makes (future) hang detection difficult
2) Does not isolate tests well
3) Precludes (future) test execution parallelization

[Dmitry Lomov]

Created attachment 93972 [details]
Restoring "running test one-by-one" logic

[Dmitry Lomov]

Created attachment 93974 [details]
Small formatting issue

[Eric Seidel (no email)]

OMG perl.

[Adam Roben (:aroben)]

Comment on attachment 93974 [details]
Small formatting issue

View in context: https://bugs.webkit.org/attachment.cgi?id=93974&action=review

> Tools/ChangeLog:8
> [phone]  Dmitry Lomov  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        run-api-tests should run one test per process
> +        https://bugs.webkit.org/show_bug.cgi?id=61088
> +
> +        * Scripts/run-api-tests:

It would be good to have some more comments in here explaining where this code came from. I assume a lot of it is from an older version of run-api-tests.

> Tools/Scripts/run-api-tests:86
> +if (runAllTests()) {
> +    exit 1;
> +}
> +else {
> +    exit 0;
> +}

"else" should be on the same line as "}". But there's no need for "else" after "exit".

> Tools/Scripts/run-api-tests:181
> +    f ($result == 0) {

"f"?

We normally say "!$result" instead of "$result == 0".

> Tools/Scripts/run-api-tests:207
> +        unless ($verbose) {
> +            open(DEVNULL, ">", File::Spec->devnull()) or die "Failed to open /dev/null";
> +            $childErr = ">&DEVNULL";
> +        } else {
> +            $childErr = ">&STDERR";
> +        }

I find "unless/else" confusing. "if/else" hurts my tiny head much less.

> Tools/Scripts/run-api-tests:297
> -
> +    

You should undo this change.

[Dmitry Lomov]

Created attachment 94113 [details]
CR feedback

[Dmitry Lomov]

Created attachment 94115 [details]
CR feedback++

[Daniel Bates]

Comment on attachment 94115 [details]
CR feedback++

View in context: https://bugs.webkit.org/attachment.cgi?id=94115&action=review

> Tools/Scripts/run-api-tests:39
> +use Term::ANSIColor qw(:constants);

Is this part of the standard Perl distribution?  From briefly googling, I came across <http://perldoc.perl.org/perlfaq8.html#How-do-I-do-fancy-stuff-with-the-keyboard%2fscreen%2fmouse%3f>, which states that it isn't. That being said, Mac OS 10 10.6.7 includes this Perl module. We should also look to see if the WebKit Cgywin distribution (for WebKit Windows development) downloads and installs this module. I take it from Adam Roben's review that this module is included in our Cgywin distribution. Even better, can we make this module optional since it only affects the visual appearance of the output?

> Tools/Scripts/run-api-tests:45
> -sub runTestTool(@);
> +sub dumpAllTests();
> +sub runAllTests();
> +sub runAllTestsInSuite($);
> +sub runTest($$);
> +sub populateTests();

Nit: Can we sort these?

> Tools/Scripts/run-api-tests:86
> +if (runAllTests()) {
> +    exit 1;
> +}
> +else {
> +    exit 0;
> +}

I would write this in one-line:

exit(runAllTests() > 0);

[Dmitry Lomov]

Comment on attachment 94115 [details]
CR feedback++

View in context: https://bugs.webkit.org/attachment.cgi?id=94115&action=review

>> Tools/Scripts/run-api-tests:39
>> +use Term::ANSIColor qw(:constants);
> 
> Is this part of the standard Perl distribution?  From briefly googling, I came across <http://perldoc.perl.org/perlfaq8.html#How-do-I-do-fancy-stuff-with-the-keyboard%2fscreen%2fmouse%3f>, which states that it isn't. That being said, Mac OS 10 10.6.7 includes this Perl module. We should also look to see if the WebKit Cgywin distribution (for WebKit Windows development) downloads and installs this module. I take it from Adam Roben's review that this module is included in our Cgywin distribution. Even better, can we make this module optional since it only affects the visual appearance of the output?

This has been there in original run-api-tests. This works on mac and and cygwin. I can remove this dependency, but maybe not in this checkin?

>> Tools/Scripts/run-api-tests:45
>> +sub populateTests();
> 
> Nit: Can we sort these?

Again, comes from original run-api-tests, but good call - I'll sort.

>> Tools/Scripts/run-api-tests:86
>> +}
> 
> I would write this in one-line:
> 
> exit(runAllTests() > 0);

Hmm, my tiny brain has a hard time reasoning about integer->boolean->integer conversions...

[Dmitry Lomov]

Created attachment 94117 [details]
CR feedback++++

[Dmitry Lomov]

Created attachment 94124 [details]
Extra bit of feedback - dealing with return code

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 94124 [details]:

java/lc3/JavaObject/JavaObjectToByte-006.html bug 60333 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 94124 [details]
Extra bit of feedback - dealing with return code

Clearing flags on attachment: 94124

Committed r86907: <http://trac.webkit.org/changeset/86907>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 94124 [details]:

http/tests/inspector/console-websocket-error.html bug 57392 (authors: [e-mail] and [e-mail])
The commit-queue is continuing to process your patch.