[webkit-reviews] review denied: [Bug 75065] run-api-tests should be able to run individual suites and tests : [Attachment 120275] Patch 4 of 5

[bugzilla-daemon at webkit.org]

Adam Roben (:aroben) <aroben at apple.com> has denied David Kilzer (ddkilzer)
<ddkilzer at webkit.org>'s request for review:
Bug 75065: run-api-tests should be able to run individual suites and tests
https://bugs.webkit.org/show_bug.cgi?id=75065

Attachment 120275: Patch 4 of 5
https://bugs.webkit.org/attachment.cgi?id=120275&action=review

------- Additional Comments from Adam Roben (:aroben) <aroben at apple.com>
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.