[Webkit-unassigned] [Bug 185783] test262/Runner.pm: add unit tests

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=185783

Daniel Bates <dbates at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #341105|review?                     |review-
              Flags|                            |

--- 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-failing-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-failing.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-passing.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-failing.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.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20180523/45a4acc7/attachment.html>