[webkit-reviews] review denied: [Bug 185783] test262/Runner.pm: add unit tests : [Attachment 341105] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed May 23 13:34:20 PDT 2018


Daniel Bates <dbates at webkit.org> has denied valerie <valerie at bocoup.com>'s
request for review:
Bug 185783: test262/Runner.pm: add unit tests
https://bugs.webkit.org/show_bug.cgi?id=185783

Attachment 341105: Patch

https://bugs.webkit.org/attachment.cgi?id=341105&action=review




--- Comment #29 from Daniel Bates <dbates at webkit.org> ---
Comment on attachment 341105
  --> https://bugs.webkit.org/attachment.cgi?id=341105
Patch

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

>
Tools/Scripts/webkitperl/test262_unittest/fixtures/test/expected-to-fail-now-fa
iling-with-new-error.js:2
> +// This code is governed by the BSD license found in the LICENSE file.

Where is this LICENSE file located? We have so more LICENSE files in the tree.
Is it necessary to reference a LICENSE file? I suggest that that put the
license inline in each file as it leaves no ambiguity as to the license terms
of each file.

>
Tools/Scripts/webkitperl/test262_unittest/fixtures/test/expected-to-fail-now-fa
iling.js:2
> +// This code is governed by the BSD license found in the LICENSE file.

Ditto.

>
Tools/Scripts/webkitperl/test262_unittest/fixtures/test/expected-to-fail-now-pa
ssing.js:2
> +// This code is governed by the BSD license found in the LICENSE file.

Ditto.

>
Tools/Scripts/webkitperl/test262_unittest/fixtures/test/expected-to-pass-now-fa
iling.js:2
> +// This code is governed by the BSD license found in the LICENSE file.

Ditto.

> Tools/Scripts/webkitperl/test262_unittest/fixtures/test/fail.js:2
> +// This code is governed by the BSD license found in the LICENSE file.

Ditto.

> Tools/Scripts/webkitperl/test262_unittest/fixtures/test/pass.js:2
> +// This code is governed by the BSD license found in the LICENSE file.

Ditto.

> Tools/Scripts/webkitperl/test262_unittest/test262-runner-tests.pl:41
> +# This test should not be run on Windows

This comment is not helpful because it just explains what the code is doing. A
good comment explain 'why' the code does a particular action. Please explain
why we do not run these tests under Windows.

> Tools/Scripts/webkitperl/test262_unittest/test262-runner-tests.pl:46
> +if ($^O eq 'MSWin32' || $^O eq 'cygwin' || $^O eq 'WinCairo') {
> +    plan(tests => 1);
> +    is(1, 1, 'do nothing for Windows builds.');
> +    exit 0;
> +}

We should import webkitdirs.pm and make use of the convenience functions
isWindows(), isCygwin(), and isWinCairo() instead of duplicating functionality
in a less readable fashion.

> Tools/Scripts/webkitperl/test262_unittest/test262-runner-tests.pl:48
> +my $Bin = $FindBin::Bin;

I do not see the benefit of this variable and it goes against our convention of
using $FindBin::Bin directly in our Perl code.

> Tools/Scripts/webkitperl/test262_unittest/test262-runner-tests.pl:50
> +my $runner = abs_path("$Bin/../../test262-runner");
> +my $mockTest262 = abs_path("$Bin/fixtures");

Please move these variables as close as possible to the code that uses them.

> Tools/Scripts/webkitperl/test262_unittest/test262-runner-tests.pl:127
> +my ($expectationsfh, $expectationsfile) = tempfile();

We follow the WebKit Code Style Guidelines for Perl code as best as we can. In
particular, variable names should be in CamelCase. The variable names
$expectationsfh, $expectationsfile do not conform to these guidelines.

> Tools/Scripts/webkitperl/test262_unittest/test262-runner-tests.pl:130
> +my $cmd = qq($runner --release --save --ignore-expectations $test262loc
$test $expect);

How did you come to the decision to hardcode --release?

> Tools/Scripts/webkitperl/test262_unittest/test262-runner-tests.pl:133
> +my $expectedexpectationsfile = "$Bin/fixtures/expectations-compare.yaml";

The name $expectedexpectationsfile does not conform to the style guidelines.


More information about the webkit-reviews mailing list