Bug 75065 – run-api-tests should be able to run individual suites and tests

• Component: Tools / Tests
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

run-api-tests currently runs all tests all the time.  It would be useful if it could run specific suites of tests and/or specific tests on a given invocation.

[David Kilzer (:ddkilzer)]

Created attachment 120272 [details]
Patch 1 of 5

[David Kilzer (:ddkilzer)]

Created attachment 120273 [details]
Patch 2 of 5

[David Kilzer (:ddkilzer)]

Created attachment 120274 [details]
Patch 3 of 5

[David Kilzer (:ddkilzer)]

Created attachment 120275 [details]
Patch 4 of 5

[David Kilzer (:ddkilzer)]

Created attachment 120276 [details]
Patch 5 of 5

[Adam Roben (:aroben)]

Comment on attachment 120275 [details]
Patch 4 of 5

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

> Tools/Scripts/run-api-tests:79
> -    'build!' => \$build
> +    'build!' => \$build,
> +    'suite=s@' => \$suiteNames

This might be a little clearer if you did this:

my @suiteNames;
...
    'suite=s' => \@suiteNames

Supposedly that means the same thing as what you did, except that then you get to deal with an array instead of an array reference in your global variable.

I'd put a trailing comma at the end of this hash so that we don't have to keep touching the last line as we add more options.

> Tools/Scripts/run-api-tests:95
>  if ($dumpTests) {
> -    dumpTestsBySuite(%testsToRun);
> +    dumpTestsBySuite(%allTests);
>      exit 0;
>  }

Should we filter based on --suite here, too? Seems like we should.

> Tools/Scripts/run-api-tests:110
> +my $failed;
> +if ($suiteNames) {
> +    my %tests;
> +    for my $suite (@$suiteNames) {
> +        if (exists $allTests{$suite}) {
> +            $tests{$suite} = $allTests{$suite};
> +        } else {
> +            warn "Suite '$suite' does not exist!";
> +        }
> +    }
> +    $failed = runTestsBySuite(%tests, $verbose);
> +} else {
> +    $failed = runTestsBySuite(%allTests, $verbose);
>  }

Maybe this would be clearer if you did something like:

my @suitesToRun = @$suiteNames or keys %allTests;
my %testsToRun;
for my $suite (@suitesToRun) {
    ...
}
exit runTestsBySuite(%testsToRun, $verbose);

That way you'd have more shared code. It might make integrating the --dump flag easier, too.

[Adam Roben (:aroben)]

Comment on attachment 120276 [details]
Patch 5 of 5

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

r+/cq- since this will likely change based on how you revise patch 4.

> Tools/Scripts/run-api-tests:79
> -    'suite=s@' => \$suiteNames
> +    'suite=s@' => \$suiteNames,
> +    'test=s@' => \$testNames

Same comments as before about using an array and a trailing comma.

> Tools/Scripts/run-api-tests:120
> -if ($suiteNames) {
> +if ($suiteNames || $testNames) {
>      my %tests;
> -    for my $suite (@$suiteNames) {
> -        if (exists $allTests{$suite}) {
> -            $tests{$suite} = $allTests{$suite};
> -        } else {
> -            warn "Suite '$suite' does not exist!";
> +    if ($suiteNames) {
> +        for my $suite (@$suiteNames) {
> +            if (exists $allTests{$suite}) {
> +                $tests{$suite} = $allTests{$suite};
> +            } else {
> +                warn "Suite '$suite' does not exist!";
> +            }
> +        }
> +    }
> +    if ($testNames) {
> +        for my $testName (@$testNames) {
> +            my ($suite, $test) = split(/\./, $testName);
> +            if (!exists $allTests{$suite}) {
> +                warn "Suite '$suite' does not exist!";
> +            } elsif (!grep($test, @{$allTests{$suite}})) {
> +                warn "Test '$testName' in suite '$suite' does not exist!";
> +            } else {
> +                push(@{$tests{$suite}}, $test);
> +            }
>          }
>      }

It seems like this would all be a lot simpler if we stored "suite.test" names in an array instead of the %allTests hash. Then you could just use grep for everything!

[Adam Roben (:aroben)]

Nice patch series! Very easy to review.

[WebKit Review Bot]

Comment on attachment 120272 [details]
Patch 1 of 5

Clearing flags on attachment: 120272

Committed r103545: <http://trac.webkit.org/changeset/103545>

[WebKit Review Bot]

Comment on attachment 120274 [details]
Patch 3 of 5

Rejecting attachment 120274 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
 exit_code: 1

Parsed 2 diffs from patch file(s).
patching file Tools/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Tools/Scripts/run-api-tests
Hunk #1 FAILED at 41.
Hunk #2 succeeded at 93 (offset 1 line).
Hunk #3 FAILED at 113.
Hunk #4 succeeded at 157 (offset 14 lines).
2 out of 4 hunks FAILED -- saving rejects to file Tools/Scripts/run-api-tests.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Adam Roben', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/10996523

[WebKit Review Bot]

Comment on attachment 120273 [details]
Patch 2 of 5

Rejecting attachment 120273 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
ngeLog
CONFLICT (content): Merge conflict in Tools/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 TestWebKitAPI: initialize the main thread before running tests

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 158.

Full output: http://queues.webkit.org/results/10997528

[WebKit Review Bot]

Comment on attachment 120274 [details]
Patch 3 of 5

Rejecting attachment 120274 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
 exit_code: 1

Parsed 2 diffs from patch file(s).
patching file Tools/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Tools/Scripts/run-api-tests
Hunk #1 FAILED at 41.
Hunk #2 succeeded at 93 (offset 1 line).
Hunk #3 FAILED at 113.
Hunk #4 succeeded at 157 (offset 14 lines).
2 out of 4 hunks FAILED -- saving rejects to file Tools/Scripts/run-api-tests.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Adam Roben', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/10999521

[David Kilzer (:ddkilzer)]

Comment on attachment 120273 [details]
Patch 2 of 5

Clearing flags on attachment: 120273

Committed r103549: <http://trac.webkit.org/changeset/103549>

[David Kilzer (:ddkilzer)]

Comment on attachment 120274 [details]
Patch 3 of 5

Clearing flags on attachment: 120274

Committed r103551: <http://trac.webkit.org/changeset/103551>

[David Kilzer (:ddkilzer)]

Created attachment 121341 [details]
Patch 4 of 5 v2

[David Kilzer (:ddkilzer)]

Created attachment 121342 [details]
Patch 5 of 5 v2

[David Kilzer (:ddkilzer)]

(In reply to comment #6)
> (From update of attachment 120275 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=120275&action=review
> 
> > Tools/Scripts/run-api-tests:95
> >  if ($dumpTests) {
> > -    dumpTestsBySuite(%testsToRun);
> > +    dumpTestsBySuite(%allTests);
> >      exit 0;
> >  }
> 
> Should we filter based on --suite here, too? Seems like we should.

Patch 5 of 5 v2 now filters when using --dump as well.


(In reply to comment #7)
> (From update of attachment 120276 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=120276&action=review
> 
> It seems like this would all be a lot simpler if we stored "suite.test" names in an array instead of the %allTests hash. Then you could just use grep for everything!

This is what Patch 4 of 5 v2 does.  I think the results are much nicer, and instead of forcing users to specify multiple, separate --suite and --test arguments (which is so tedious anyway), you can now simply list the suites and tests that you want to run on the command-line:

$ ./Tools/Scripts/run-api-tests --no-build --dump WebKit1 WTF_Vector RetainPtr.AdoptNS
Dumping test cases
------------------
RetainPtr:
   AdoptNS
WTF_Vector:
   Iterator
   ReverseIterator
   ReversedProxy
WebKit1:
   DOMRangeOfString
   RenderedImageFromDOMRange
   StringByEvaluatingJavaScriptFromString
   SubresourceErrorCrash
------------------

[Adam Roben (:aroben)]

> (In reply to comment #7)
> > (From update of attachment 120276 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=120276&action=review
> > 
> > It seems like this would all be a lot simpler if we stored "suite.test" names in an array instead of the %allTests hash. Then you could just use grep for everything!
> 
> This is what Patch 4 of 5 v2 does.  I think the results are much nicer, and instead of forcing users to specify multiple, separate --suite and --test arguments (which is so tedious anyway), you can now simply list the suites and tests that you want to run on the command-line:
> 
> $ ./Tools/Scripts/run-api-tests --no-build --dump WebKit1 WTF_Vector RetainPtr.AdoptNS

Nice!

[Adam Roben (:aroben)]

Comment on attachment 121341 [details]
Patch 4 of 5 v2

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

> Tools/Scripts/run-api-tests:117
> +            print $suite . ":\n";
> +        }
> +        print "   " . $test . "\n";

In both of these cases, I'd either use string interpolation:

print "    $test\n";

or multiple arguments to print:

print "    ", $test, "\n";

Explicit concatenation seems a little funny.

> Tools/Scripts/run-api-tests:136
> +        if ($failed) {
> +            $anyFailures = 1;
>          }

I know this isn't new code, but it seems like we could just say $anyFailures ||= $failed.

> Tools/Scripts/run-api-tests:333
> +sub sortTestList($$)
> +{
> +    my ($a, $b) = @_;
> +    my ($suiteA, $testA) = split(/\./, $a);
> +    my ($suiteB, $testB) = split(/\./, $b);
> +    return $suiteA cmp $suiteB ? $suiteA cmp $suiteB : $testA cmp $testB;
> +}

How is this different from the standard sort?

|| in Perl returns the value of the expression that was true-ish, so you can just say:

return $suiteA cmp $suiteB || $testA cmp $testB;

(Assuming cmp has higher precedence than ||.)

[Adam Roben (:aroben)]

Comment on attachment 121342 [details]
Patch 5 of 5 v2

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

> Tools/Scripts/run-api-tests:139
> +sub filterTests(\@\@)
> +{
> +    my ($tests, $filterList) = @_;
> +    my %suiteNames;
> +    my %testNames;
> +
> +    foreach my $filter (@$filterList) {
> +        if ($filter =~ m/\./) {
> +            $testNames{$filter} = 1;
> +        } else {
> +            $suiteNames{$filter} = 1;
> +        }
> +    }
> +
> +    my @filteredTests = grep {
> +        my $suite = (split(/\./, $_))[0];
> +        exists $suiteNames{$suite} || exists $testNames{$_};
> +    } @$tests;
> +
> +    return @filteredTests;
> +}

Maybe we don't need to be so strict. The strings passed on the command line could just be treated as prefixes of fully-qualified test names. Any test that starts with a prefix that was specified on the command line will be run. That would allow someone to specify "MetaAllocatorTest.Repeat" and have all the tests that start with that prefix run, or to specify "RetainPtr" and have both the RetainPtr and RetainPtrHashing suites run. (If they just want the RetainPtr suite to run they can say "RetainPtr.".) It would probably allow a slightly simpler implementation too.

[David Kilzer (:ddkilzer)]

(In reply to comment #19)
> (From update of attachment 121341 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=121341&action=review
> 
> > Tools/Scripts/run-api-tests:117
> > +            print $suite . ":\n";
> > +        }
> > +        print "   " . $test . "\n";
> 
> In both of these cases, I'd either use string interpolation:
> 
> print "    $test\n";
> 
> or multiple arguments to print:
> 
> print "    ", $test, "\n";
> 
> Explicit concatenation seems a little funny.

This was pre-existing, but I can fix it.  (I actually considered it, but didn't want to get caught doing too much clean-up. :)

> > Tools/Scripts/run-api-tests:136
> > +        if ($failed) {
> > +            $anyFailures = 1;
> >          }
> 
> I know this isn't new code, but it seems like we could just say $anyFailures ||= $failed.

Will do.

> > Tools/Scripts/run-api-tests:333
> > +sub sortTestList($$)
> > +{
> > +    my ($a, $b) = @_;
> > +    my ($suiteA, $testA) = split(/\./, $a);
> > +    my ($suiteB, $testB) = split(/\./, $b);
> > +    return $suiteA cmp $suiteB ? $suiteA cmp $suiteB : $testA cmp $testB;
> > +}
> 
> How is this different from the standard sort?

It's sorting by suite, then test.  I wasn't sure whether sorting alphabetically using the whole "SuiteName.TestName" string would work.  (Looking at "man ascii", though, "." comes before all letters, numbers and underscore, so we probably don't need this.

> || in Perl returns the value of the expression that was true-ish, so you can just say:
> 
> return $suiteA cmp $suiteB || $testA cmp $testB;
> 
> (Assuming cmp has higher precedence than ||.)

For some reason, I was thinking "-1 || 1" would evaluate to "true" instead of "-1".  I think I can simply remove the custom sorting function like you said.


(In reply to comment #20)
> (From update of attachment 121342 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=121342&action=review
> 
> > Tools/Scripts/run-api-tests:139
> > +sub filterTests(\@\@)
> > +{
> > +    my ($tests, $filterList) = @_;
> > +    my %suiteNames;
> > +    my %testNames;
> > +
> > +    foreach my $filter (@$filterList) {
> > +        if ($filter =~ m/\./) {
> > +            $testNames{$filter} = 1;
> > +        } else {
> > +            $suiteNames{$filter} = 1;
> > +        }
> > +    }
> > +
> > +    my @filteredTests = grep {
> > +        my $suite = (split(/\./, $_))[0];
> > +        exists $suiteNames{$suite} || exists $testNames{$_};
> > +    } @$tests;
> > +
> > +    return @filteredTests;
> > +}
> 
> Maybe we don't need to be so strict. The strings passed on the command line could just be treated as prefixes of fully-qualified test names. Any test that starts with a prefix that was specified on the command line will be run. That would allow someone to specify "MetaAllocatorTest.Repeat" and have all the tests that start with that prefix run, or to specify "RetainPtr" and have both the RetainPtr and RetainPtrHashing suites run. (If they just want the RetainPtr suite to run they can say "RetainPtr.".) It would probably allow a slightly simpler implementation too.

I was wondering about doing prefix searches, but I didn't realize you could limit the search to a single suite by appending a "." on the end of the name.  "WTF" would find both "WTF" and WTF_Vector", but "WTF." would just match the tests in the "WTF" suite.  This seems more flexible (if slightly more obscure), so I'll switch to doing that instead.

Thanks!

[David Kilzer (:ddkilzer)]

Committed r104342: <http://trac.webkit.org/changeset/104342>

[David Kilzer (:ddkilzer)]

Committed r104343: <http://trac.webkit.org/changeset/104343>