Summary: | run-api-tests should be able to run individual suites and tests | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | David Kilzer (:ddkilzer) <ddkilzer> | ||||||||||||||||
Component: | Tools / Tests | Assignee: | David Kilzer (:ddkilzer) <ddkilzer> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | aroben, sam, webkit.review.bot | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Attachments: |
|
Description
David Kilzer (:ddkilzer)
2011-12-21 21:59:56 PST
Created attachment 120272 [details]
Patch 1 of 5
Created attachment 120273 [details]
Patch 2 of 5
Created attachment 120274 [details]
Patch 3 of 5
Created attachment 120275 [details]
Patch 4 of 5
Created attachment 120276 [details]
Patch 5 of 5
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. 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! Nice patch series! Very easy to review. Comment on attachment 120272 [details] Patch 1 of 5 Clearing flags on attachment: 120272 Committed r103545: <http://trac.webkit.org/changeset/103545> 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 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 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 Comment on attachment 120273 [details] Patch 2 of 5 Clearing flags on attachment: 120273 Committed r103549: <http://trac.webkit.org/changeset/103549> Comment on attachment 120274 [details] Patch 3 of 5 Clearing flags on attachment: 120274 Committed r103551: <http://trac.webkit.org/changeset/103551> Created attachment 121341 [details]
Patch 4 of 5 v2
Created attachment 121342 [details]
Patch 5 of 5 v2
(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 ------------------ > (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!
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 ||.) 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. (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! Committed r104342: <http://trac.webkit.org/changeset/104342> Committed r104343: <http://trac.webkit.org/changeset/104343> |