Bug 185783 – test262/Runner.pm: add unit tests

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[valerie]

Add unit tests for test262/Runner.pm

[valerie]

Created attachment 340736 [details]
Patch

[Jonathan Bedard]

Comment on attachment 340736 [details]
Patch

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

> Tools/ChangeLog:11
> +        (main):

It seems worth explaining why changes are required here.

> Tools/Scripts/test262/Runner.pm:265
> +    if ($Mode ne 'TESTING') { # if testing, no need to load harness files

Is there any harm in loading harness files while testing?.  We aren't really resource constrained in webkitperl tests.

> Tools/Scripts/webkitperl/test262_unittest/fixtures/test/expected-to-fail-now-failing-with-new-error.js:8
> +throw "Test262Unexpected: This test errors WITH A UNEXPECTED ERROR.";

Nit: Change to 'Test262Unexpected: This test fails WITH AN UNEXPECTED ERROR.'

> Tools/Scripts/webkitperl/test262_unittest/fixtures/test/expected-to-fail-now-failing.js:8
> +throw "Test262: This test errors.";

Nit: Change to 'Test262: This test fails.'

> Tools/Scripts/webkitperl/test262_unittest/fixtures/test/expected-to-pass-now-failing.js:8
> +throw "Test262: This test errors.";

Nit: Change to 'Test262: This test fails.'

> Tools/Scripts/webkitperl/test262_unittest/fixtures/test/fail.js:8
> +throw "Test262: This test errors.";

Nit: Change to 'Test262: This test fails.'

> Tools/Scripts/webkitperl/test262_unittest/test262-runner-tests.pl:42
> +if ($^O eq 'MSWin32' && $^O eq 'cygwin') {

Shouldn't this be or?  I believe that we have a non-cygwin windows port.  (WinCairo)

[valerie]

(In reply to Jonathan Bedard from comment #2)

> > Tools/Scripts/test262/Runner.pm:265
> > +    if ($Mode ne 'TESTING') { # if testing, no need to load harness files
> 
> Is there any harm in loading harness files while testing?.  We aren't really
> resource constrained in webkitperl tests.

These tests supply "--test262" command line option which points to a "fake" test262 in the directory "webkitperl/test262-unittest/fixture". The harness directory is expect to be found in the directory supplied by "--test262", but I didn't want to copy over the harness files here (it would be another thing that could easily get out of date).

Do you think it would be better to copy over the expected harness files, or to silently ignore them if they don't exist, rather than have this environment option?

[valerie]

Created attachment 340867 [details]
Patch

[EWS Watchlist]

Comment on attachment 340867 [details]
Patch

Attachment 340867 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/7761491

New failing tests:
http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-audio.html

[EWS Watchlist]

Created attachment 340957 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Jonathan Bedard]

(In reply to valerie from comment #3)
> (In reply to Jonathan Bedard from comment #2)
> 
> > > Tools/Scripts/test262/Runner.pm:265
> > > +    if ($Mode ne 'TESTING') { # if testing, no need to load harness files
> > 
> > Is there any harm in loading harness files while testing?.  We aren't really
> > resource constrained in webkitperl tests.
> 
> These tests supply "--test262" command line option which points to a "fake"
> test262 in the directory "webkitperl/test262-unittest/fixture". The harness
> directory is expect to be found in the directory supplied by "--test262",
> but I didn't want to copy over the harness files here (it would be another
> thing that could easily get out of date).
> 
> Do you think it would be better to copy over the expected harness files, or
> to silently ignore them if they don't exist, rather than have this
> environment option?

So if I'm understanding this correctly, the directory directory that --test262 is replacing contains both the harness and testing files in production?

It seems to me, then, that either the harness files shouldn't live with the testing files or that we should fallback to the production harness files if there are not harness files in the testing directory.  This would allow you to eliminate the whole 'testing' directory idea.

[valerie]

Created attachment 341002 [details]
Patch

[valerie]

(In reply to Jonathan Bedard from comment #7)

> It seems to me, then, that either the harness files shouldn't live with the
> testing files or that we should fallback to the production harness files if
> there are not harness files in the testing directory.  This would allow you
> to eliminate the whole 'testing' directory idea.

I think that is the best idea and I've now implemented it -- patch uploaded. The environment variable is removed, and, if the harness files are not found within the supplied test262 directory, then the script will default to webkit's test262 directory.

The reason the harness files are alongside the test files is because Webkit's test262 directory matches the directory structure of the test262 git repo. If you have a checkout of the test262 repo, you can point the runner your local checkout with --t262 and it will run the tests (with the harness files) there.

Finally, the diff of Runner.pm might be a little confusing because I moved around some unrelated logic. I found it pretty obnoxious that the test262-results directory is not lazily created when running this script, so that directory is now lazily created with this patch.

[Jonathan Bedard]

Comment on attachment 341002 [details]
Patch

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

> Tools/Scripts/test262/Runner.pm:273
> +    my @defaultharnessfiles = (

Nit: Should be camel cased.

> Tools/Scripts/webkitperl/test262_unittest/test262-runner-tests.pl:48
> +my $ToolsPath = $ENV{'WEBKIT_LIBRARIES'};

Nit: Not a class, should start with a lower-case T

Also, I'm pretty sure this isn't the right way to do this.  Just tried applying your patch and running locally on my machine (via test-webkitperl) I see errors like this:
Can't exec "/Tools/Scripts/test262-runner": No such file or directory at Tools/Scripts/webkitperl/test262_unittest/test262-runner-tests.pl line 132.

> Tools/Scripts/webkitperl/test262_unittest/test262-runner-tests.pl:49
> +my $Runner = File::Spec->catfile($ToolsPath, 'Tools', 'Scripts', 'test262-runner');

Nit: Not a class, should start with a lower-case R

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

Nit: Not a class, should start with a lower-case M

[valerie]

(In reply to Jonathan Bedard from comment #10)

> > Tools/Scripts/webkitperl/test262_unittest/test262-runner-tests.pl:48
> > +my $ToolsPath = $ENV{'WEBKIT_LIBRARIES'};
> 
> Nit: Not a class, should start with a lower-case T
> 
> Also, I'm pretty sure this isn't the right way to do this.  Just tried
> applying your patch and running locally on my machine (via test-webkitperl)
> I see errors like this:
> Can't exec "/Tools/Scripts/test262-runner": No such file or directory at
> Tools/Scripts/webkitperl/test262_unittest/test262-runner-tests.pl line 132.
> 

I copied this from other tests in webkitperl. I also set WEBKIT_LIBRARIES in my bash scripts -- I thought it might be standard? Perhaps not if you haven't heard of it. 

Can you think of a better way to do it, or should I just keep this (as it's at least consistent with a couple other webkit perl scripts?

I originally wanted to use webkitperl.pm, but that module doesn't expose `determineSourceDir` -- the function to get the root of the webkit directory repo.

[Jonathan Bedard]

(In reply to valerie from comment #11)
> (In reply to Jonathan Bedard from comment #10)
> 
> > > Tools/Scripts/webkitperl/test262_unittest/test262-runner-tests.pl:48
> > > +my $ToolsPath = $ENV{'WEBKIT_LIBRARIES'};
> > 
> > Nit: Not a class, should start with a lower-case T
> > 
> > Also, I'm pretty sure this isn't the right way to do this.  Just tried
> > applying your patch and running locally on my machine (via test-webkitperl)
> > I see errors like this:
> > Can't exec "/Tools/Scripts/test262-runner": No such file or directory at
> > Tools/Scripts/webkitperl/test262_unittest/test262-runner-tests.pl line 132.
> > 
> 
> I copied this from other tests in webkitperl. I also set WEBKIT_LIBRARIES in
> my bash scripts -- I thought it might be standard? Perhaps not if you
> haven't heard of it. 
> 
> Can you think of a better way to do it, or should I just keep this (as it's
> at least consistent with a couple other webkit perl scripts?
> 
> I originally wanted to use webkitperl.pm, but that module doesn't expose
> `determineSourceDir` -- the function to get the root of the webkit directory
> repo.

When we do these sorts of things in Python and bash, we will try and make the module self contained by referencing outside resources relative to the module file.  That seems like the correct approach here.  For Perl, that would be dirname(__FILE__).

[valerie]

Created attachment 341027 [details]
Patch

[valerie]

Created attachment 341028 [details]
Patch

[valerie]

Ok, since I can references things relatively -- can you take a look and try to run now?

[Jonathan Bedard]

Ok, things run locally (and pass!) for me.

Unofficial r+ from me, but I need to ping some WebKit reviewers so you can get this landed.

[WebKit Commit Bot]

Comment on attachment 341028 [details]
Patch

Clearing flags on attachment: 341028

Committed r232112: <https://trac.webkit.org/changeset/232112>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

It looks like tests are failing on the bots:
https://build.webkit.org/builders/Apple%20High%20Sierra%20Release%20WK1%20%28Tests%29/builds/5548 

Cannot find jsc, try with --release or specify with --jsc <path>.

Compilation failed in require at /Volumes/Data/slave/sierra-release-tests-wk1/build/Tools/Scripts/test262-runner line 45.
BEGIN failed--compilation aborted at /Volumes/Data/slave/sierra-release-tests-wk1/build/Tools/Scripts/test262-runner line 45.

#   Failed test 'test262 test failed, ignore expectations (new failures: 2)'
#   at Tools/Scripts/webkitperl/test262_unittest/test262-runner-tests.pl line 121.
#          got: '0'
#     expected: '2'
Cannot find jsc, try with --release or specify with --jsc <path>.

Compilation failed in require at /Volumes/Data/slave/sierra-release-tests-wk1/build/Tools/Scripts/test262-runner line 45.
BEGIN failed--compilation aborted at /Volumes/Data/slave/sierra-release-tests-wk1/build/Tools/Scripts/test262-runner line 45.

#   Failed test 'test262 test passed, ignore expectations (exit code: 0)'
#   at Tools/Scripts/webkitperl/test262_unittest/test262-runner-tests.pl line 115.
#          got: '1'
#     expected: '0'
Cannot find jsc, try with --release or specify with --jsc <path>.

Compilation failed in require at /Volumes/Data/slave/sierra-release-tests-wk1/build/Tools/Scripts/test262-runner line 45.
BEGIN failed--compilation aborted at /Volumes/Data/slave/sierra-release-tests-wk1/build/Tools/Scripts/test262-runner line 45.

#   Failed test 'test262 tests newly failed (new failures: 2)'
#   at Tools/Scripts/webkitperl/test262_unittest/test262-runner-tests.pl line 121.
#          got: '0'
#     expected: '2'
Cannot find jsc, try with --release or specify with --jsc <path>.

Compilation failed in require at /Volumes/Data/slave/sierra-release-tests-wk1/build/Tools/Scripts/test262-runner line 45.
BEGIN failed--compilation aborted at /Volumes/Data/slave/sierra-release-tests-wk1/build/Tools/Scripts/test262-runner line 45.

#   Failed test 'test262 tests newly passed (exit code: 0)'
#   at Tools/Scripts/webkitperl/test262_unittest/test262-runner-tests.pl line 115.
#          got: '1'
#     expected: '0'
Cannot find jsc, try with --release or specify with --jsc <path>.

Compilation failed in require at /Volumes/Data/slave/sierra-release-tests-wk1/build/Tools/Scripts/test262-runner line 45.
BEGIN failed--compilation aborted at /Volumes/Data/slave/sierra-release-tests-wk1/build/Tools/Scripts/test262-runner line 45.

#   Failed test 'test262 tests fails, expected failure (exit code: 0)'
#   at Tools/Scripts/webkitperl/test262_unittest/test262-runner-tests.pl line 115.
#          got: '1'
#     expected: '0'
Cannot find jsc, try with --release or specify with --jsc <path>.

Compilation failed in require at /Volumes/Data/slave/sierra-release-tests-wk1/build/Tools/Scripts/test262-runner line 45.
BEGIN failed--compilation aborted at /Volumes/Data/slave/sierra-release-tests-wk1/build/Tools/Scripts/test262-runner line 45.

#   Failed test 'test262 tests fails, with unexpected error string (new failures: 2)'
#   at Tools/Scripts/webkitperl/test262_unittest/test262-runner-tests.pl line 121.
#          got: '0'
#     expected: '2'
Cannot find jsc, try with --release or specify with --jsc <path>.

Compilation failed in require at /Volumes/Data/slave/sierra-release-tests-wk1/build/Tools/Scripts/test262-runner line 45.
BEGIN failed--compilation aborted at /Volumes/Data/slave/sierra-release-tests-wk1/build/Tools/Scripts/test262-runner line 45.

#   Failed test 'expectations yaml file format'
#   at Tools/Scripts/webkitperl/test262_unittest/test262-runner-tests.pl line 135.
# Looks like you failed 7 tests of 13.

[Ryan Haddad]

Reverted r232112 for reason:

The tests added with this change  are failing on the bots.

Committed r232115: <https://trac.webkit.org/changeset/232115>

[valerie]

Created attachment 341105 [details]
Patch

[valerie]

(In reply to Ryan Haddad from comment #19)
> It looks like tests are failing on the bots:
> https://build.webkit.org/builders/
> Apple%20High%20Sierra%20Release%20WK1%20%28Tests%29/builds/5548 
> 
> Cannot find jsc, try with --release or specify with --jsc <path>.

It's possible the patch I uploaded fixed this problem, but I won't know until it is run on buildbot.

Would could I ask to find the location of jsc in the buildbot environment?

[Ryan Haddad]

(In reply to valerie from comment #22)
> (In reply to Ryan Haddad from comment #19)
> > It looks like tests are failing on the bots:
> > https://build.webkit.org/builders/
> > Apple%20High%20Sierra%20Release%20WK1%20%28Tests%29/builds/5548 
> > 
> > Cannot find jsc, try with --release or specify with --jsc <path>.
> 
> It's possible the patch I uploaded fixed this problem, but I won't know
> until it is run on buildbot.
> 
> Would could I ask to find the location of jsc in the buildbot environment?
It should be something like:
/Volumes/Data/slave/sierra-release-tests-wk1/build/WebKitBuild/Release/jsc

[valerie]

(In reply to Ryan Haddad from comment #23)
> (In reply to valerie from comment #22)
> > (In reply to Ryan Haddad from comment #19)
> > > It looks like tests are failing on the bots:
> > > https://build.webkit.org/builders/
> > > Apple%20High%20Sierra%20Release%20WK1%20%28Tests%29/builds/5548 
> > > 
> > > Cannot find jsc, try with --release or specify with --jsc <path>.
> > 
> > It's possible the patch I uploaded fixed this problem, but I won't know
> > until it is run on buildbot.
> > 
> > Would could I ask to find the location of jsc in the buildbot environment?
> It should be something like:
> /Volumes/Data/slave/sierra-release-tests-wk1/build/WebKitBuild/Release/jsc

Thanks Ryan -- I think the patch I just uploaded will work. It specifies to look for the release build instead of the debug build (which it was previously doing). Could you possible review and commit queue it?

[Ryan Haddad]

(In reply to valerie from comment #24)
> Thanks Ryan -- I think the patch I just uploaded will work. It specifies to
> look for the release build instead of the debug build (which it was
> previously doing). Could you possible review and commit queue it?

The path to jsc will depend upon the configuration we are testing, so it could be either release or debug.

[Jonathan Bedard]

(In reply to valerie from comment #24)
> ...
> 
> Thanks Ryan -- I think the patch I just uploaded will work. It specifies to
> look for the release build instead of the debug build (which it was
> previously doing). Could you possible review and commit queue it?

That won't work everywhere in infrastructure.

You should have a fallback to system jsc (using which jsc) too.  I assume that you aren't intending to test the jsc binary, just the test262 scripts, right?

[valerie]

(In reply to Jonathan Bedard from comment #26)
> (In reply to valerie from comment #24)
> > ...
> > 
> > Thanks Ryan -- I think the patch I just uploaded will work. It specifies to
> > look for the release build instead of the debug build (which it was
> > previously doing). Could you possible review and commit queue it?
> 
> That won't work everywhere in infrastructure.
> 
> You should have a fallback to system jsc (using which jsc) too.  I assume
> that you aren't intending to test the jsc binary, just the test262 scripts,
> right?

Right, it's not intending to test jsc binary. It tests the test infrastructure for testing the jsc binary.

If the runner cannot find jsc using webkitdirs.pm, it will try to find the system jsc (viw which jsc)! So unfortunately, jsc is not in the path on this buildbot slave.

[valerie]

(In reply to Ryan Haddad from comment #25)
> (In reply to valerie from comment #24)
> > Thanks Ryan -- I think the patch I just uploaded will work. It specifies to
> > look for the release build instead of the debug build (which it was
> > previously doing). Could you possible review and commit queue it?
> 
> The path to jsc will depend upon the configuration we are testing, so it
> could be either release or debug.

I see. Like (In reply to Ryan Haddad from comment #25)
> (In reply to valerie from comment #24)
> > Thanks Ryan -- I think the patch I just uploaded will work. It specifies to
> > look for the release build instead of the debug build (which it was
> > previously doing). Could you possible review and commit queue it?
> 
> The path to jsc will depend upon the configuration we are testing, so it
> could be either release or debug.

How can I get the configuration, so I know whether to script should set "Release" or "Debug"?

[Daniel Bates]

Comment on attachment 341105 [details]
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.

[Jonathan Bedard]

Looking around on bots, turns out that not all Mac configurations are going to have jsc in the system path.  However, all Mac configurations will have jsc at this path: '/System/Library/Frameworks/JavaScriptCore.framework/Resources/jsc'.  For Mac, that will be the safest jsc fallback path.

[Daniel Bates]

Comment on attachment 341105 [details]
Patch

I am not happy with this patch. I do not understand the need to add tests for a runner as it seems excessive to do. With the exception of the webkitpy code we (the WebKit OpenSource Project) tends to not write tests for driver code (e.g. build-webkit, DumpRenderTree, et cetera). Instead we prefer to write tests for the framework/library/module code that the driver calls. There is a careful balancing act between adding testing to catch regressions and hindering hackability because the tests actually make the program's design more rigid. Writing tests for the driver itself (i.e. writing end-to-end integration tests) tends to hurt hack-ability more than the benefit it provides. Can we factor out more of the driver code into an existing Perl module(s) or new modules and then write tests for them instead of writing an end-to-end integration test for the driver itself? If we chose to move forward with adding tests for the driver then can we at least mock out the calls to the jsc binary and their results so that we can avoid the need to use the real jsc binary and hence get into the situation with have now of what jsc binary to use (debug/release/system)? Can we make more use of webkitdirs.pm?

[valerie]

(In reply to Daniel Bates from comment #31)
> Comment on attachment 341105 [details]
> Patch
> 
> I am not happy with this patch. I do not understand the need to add tests
> for a runner as it seems excessive to do. With the exception of the webkitpy
> code we (the WebKit OpenSource Project) tends to not write tests for driver
> code (e.g. build-webkit, DumpRenderTree, et cetera). Instead we prefer to
> write tests for the framework/library/module code that the driver calls.
> There is a careful balancing act between adding testing to catch regressions
> and hindering hackability because the tests actually make the program's
> design more rigid. Writing tests for the driver itself (i.e. writing
> end-to-end integration tests) tends to hurt hack-ability more than the
> benefit it provides.

I thought about this when writing these tests, which is why, in the end, they are pretty simple. The important thing (from my perspective) is that we get an error code/failure under the appropriate conditions. If we change the scenario on purpose, then these tests can be updated. Otherwise I hope the test will catch anyone editing the test262-harness script output without realizing the infrastructure that depends on it.

For example: someone might edit the output because they are using the harness as part of their local development practice, but the script is also used in buildbot to surface failures to jsc devs. If those edits break buildbot's ability to surface those errors, I don't want that to be silent.

> Can we factor out more of the driver code into an
> existing Perl module(s) or new modules and then write tests for them instead
> of writing an end-to-end integration test for the driver itself?

The test262-runner script had some strong motivation to be stand alone, and has a bizarre compatibility on 5.8.8 (bizarre as in nothing else in webkit does). 

> If we chose
> to move forward with adding tests for the driver then can we at least mock
> out the calls to the jsc binary and their results so that we can avoid the
> need to use the real jsc binary and hence get into the situation with have
> now of what jsc binary to use (debug/release/system)? Can we make more use
> of webkitdirs.pm?

Interesting idea. I hope my colleague Leo Balter can look into this. He'll take over future patches on this bug. As of this moment, I'm out on vacation for two weeks!

[Daniel Bates]

(In reply to valerie from comment #32)
> (In reply to Daniel Bates from comment #31)
> > Comment on attachment 341105 [details]
> > Patch
> > 
> > I am not happy with this patch. I do not understand the need to add tests
> > for a runner as it seems excessive to do. With the exception of the webkitpy
> > code we (the WebKit OpenSource Project) tends to not write tests for driver
> > code (e.g. build-webkit, DumpRenderTree, et cetera). Instead we prefer to
> > write tests for the framework/library/module code that the driver calls.
> > There is a careful balancing act between adding testing to catch regressions
> > and hindering hackability because the tests actually make the program's
> > design more rigid. Writing tests for the driver itself (i.e. writing
> > end-to-end integration tests) tends to hurt hack-ability more than the
> > benefit it provides.
> 
> I thought about this when writing these tests, which is why, in the end,
> they are pretty simple. The important thing (from my perspective) is that we
> get an error code/failure under the appropriate conditions. If we change the
> scenario on purpose, then these tests can be updated. Otherwise I hope the
> test will catch anyone editing the test262-harness script output without
> realizing the infrastructure that depends on it.
> 
> For example: someone might edit the output because they are using the
> harness as part of their local development practice, but the script is also
> used in buildbot to surface failures to jsc devs. If those edits break buildbot's ability to surface those errors, I don't want that to be silent.
> 

Buildbot unit tests are the appropriate solution to catch such regressions. The Buildbot code and unit tests are in Tools/BuildSlaveSupport/build.webkit.org-config. In particular, the unit tests for the Buildbot steps are in <https://trac.webkit.org/browser/webkit/trunk/Tools/BuildSlaveSupport/build.webkit.org-config/steps_unittest.py?rev=231570>. You may find it beneficial to look at the unit tests classes RunJavaScriptCoreTestsTest and RunWebKitTestsTest for inspiration on how to write tests for the output of the runner.

> > Can we factor out more of the driver code into an
> > existing Perl module(s) or new modules and then write tests for them instead
> > of writing an end-to-end integration test for the driver itself?
> 
> The test262-runner script had some strong motivation to be stand alone [...]
> 

You wrote "had", the past tense of "have". Was this intentional and you are expressing that there is no longer a strong motivation to be stand alone and we can use more of webkitdirs.pm? Or was this accidental and the tests262-runner still has a strong motivation to be stand alone? If the former, then I suggest we make more use of webkitdirs.pm as it will simplify the code and make it more consistent with our other Perl code. If the tests262-runner is still motivated to be stand alone then how did you come to the decision to incorporate the tests for it into the same directory hierarchy as the unit tests for all of our shared Perl code and use test-webkitperl to run them. I mean, if there is a strong motivation for the test262-runner to be stand alone then why isn't there a strong motivation to have the unit test runner for the test262-runner be stand alone? I suspect there is an argument of convenience here and re-purposing of code. I'm curious how the boundary is being drawn.

> > If we chose
> > to move forward with adding tests for the driver then can we at least mock
> > out the calls to the jsc binary and their results so that we can avoid the
> > need to use the real jsc binary and hence get into the situation with have
> > now of what jsc binary to use (debug/release/system)? Can we make more use
> > of webkitdirs.pm?
> 
> Interesting idea. I hope my colleague Leo Balter can look into this. He'll
> take over future patches on this bug. As of this moment, I'm out on vacation
> for two weeks!

See my remark above about Buildbot unit tests. Buildbot unit test seem more appropriate for your purpose.

[Daniel Bates]

(In reply to Daniel Bates from comment #33)
> 
> Buildbot unit tests are the appropriate solution to catch such regressions.
> The Buildbot code and unit tests are in
> Tools/BuildSlaveSupport/build.webkit.org-config. In particular, the unit
> tests for the Buildbot steps are in
> <https://trac.webkit.org/browser/webkit/trunk/Tools/BuildSlaveSupport/build.
> webkit.org-config/steps_unittest.py?rev=231570>. You may find it beneficial
> to look at the unit tests classes RunJavaScriptCoreTestsTest and
> RunWebKitTestsTest for inspiration on how to write tests for the output of
> the runner.
> 

Disregard this remark. Buildbot unit tests will not catch such regressions. It is still beneficial to add Buildbot unit tests to catch parsing regressions in how we "surface those errors" in the Buildbot UI.

> > > If we chose
> > > to move forward with adding tests for the driver then can we at least mock
> > > out the calls to the jsc binary and their results so that we can avoid the
> > > need to use the real jsc binary and hence get into the situation with have
> > > now of what jsc binary to use (debug/release/system)? Can we make more use
> > > of webkitdirs.pm?
> > 
> > Interesting idea. I hope my colleague Leo Balter can look into this. He'll
> > take over future patches on this bug. As of this moment, I'm out on vacation
> > for two weeks!
> 
> See my remark above about Buildbot unit tests. Buildbot unit test seem more
> appropriate for your purpose.

Disregard this remark.

[Leo Balter]

As Valerie mentioned, I'm taking over this patch as she's in a vacation time.

> It is still beneficial to add Buildbot unit tests to catch parsing regressions in how we "surface those errors" in the Buildbot UI.

The unit tests for the buildbot contains only tests for how the buildbot will interpret the results, isolating from anything else, including the results from the test262-runner. 
It contains a static provided expectation text and asserts how the buildbot script read it.

This is for me an important reason to maintain tests for the runner that asserts the output will be compatible with the buildbot, but I'm not sure why they need to be in the same place. I can do it anyway if you prefer.

> You wrote "had", the past tense of "have". Was this intentional and you are expressing that there is no longer a strong motivation to be stand alone and we can use more of webkitdirs.pm? Or was this accidental and the tests262-runner still has a strong motivation to be stand alone?

We still have a motivation but Valerie did some throughout workarounds to minimize the incompatibility.

This has been discussed through other patches and I'm afraid there are a few details I can't disclose here. Valerie already mentioned we need to keep it compatible with Perl 5.8.8 and the webkitdirs.pm is not, so it's hard to rely on it.

> If the former, then I suggest we make more use of webkitdirs.pm as it will simplify the code and make it more consistent with our other Perl code. 

I sent a patch for this already and it got rejected. We also considered it would be a unwelcome burden having to tell everyone to keep consistency for it with such an old version of Perl as it is not directly necessary for this script.

> If the tests262-runner is still motivated to be stand alone then how did you come to the decision to incorporate the tests for it into the same directory hierarchy as the unit tests for all of our shared Perl code and use test-webkitperl to run them. I mean, if there is a strong motivation for the test262-runner to be stand alone then why isn't there a strong motivation to have the unit test runner for the test262-runner be stand alone?

The decision was made to keep consistency over the other unit tests. I believe some of this also received feedback in this same patch issue.

This was finally decided through patches discussions and internal discussions with Filip Pizlo and Michael Saboff. Do you think this is worth going all the way back as a time effort considering where we the tests for the runner should be placed? I honestly don't mind of any place, but I feel I can't decide myself.

[Daniel Bates]

(In reply to Leo Balter from comment #35)
> As Valerie mentioned, I'm taking over this patch as she's in a vacation time.
> 
> > It is still beneficial to add Buildbot unit tests to catch parsing regressions in how we "surface those errors" in the Buildbot UI.
> 
> The unit tests for the buildbot contains only tests for how the buildbot
> will interpret the results, isolating from anything else, including the
> results from the test262-runner. 
> It contains a static provided expectation text and asserts how the buildbot
> script read it.
> 
> This is for me an important reason to maintain tests for the runner that
> asserts the output will be compatible with the buildbot, 

I do not think tests are the appropriate way to enforce this compatibility. End-to-end buildbot tests would, but this may not be feasible. You will need an English explanation somewhere in either the test runner or unit tests (or both) to explain that the formatting of the output must never change without changing buildbot parsing code. Otherwise, the tests themselves only assert the output was compatible with the output buildbot expected as of the some date. Lack of such English could lead to someone changing the output format AND the tests only to cause Buildbot breakage once a WebKit.org admin pushes a change to Builbot.

If we are still interested in pursuing tests for the output then can we at least mock out the calls to JSC so that we do not need to have a dependency on it?

> > If the former, then I suggest we make more use of webkitdirs.pm as it will simplify the code and make it more consistent with our other Perl code. 
> 
> I sent a patch for this already and it got rejected. We also considered it
> would be a unwelcome burden having to tell everyone to keep consistency for
> it with such an old version of Perl as it is not directly necessary for this
> script.
> 

Do you have a bug # for this?

[Leo Balter]

> I do not think tests are the appropriate way to enforce this compatibility. End-to-end buildbot tests would, but this may not be feasible. You will need an English explanation somewhere in either the test runner or unit tests (or both) to explain that the formatting of the output must never change without changing buildbot parsing code. Otherwise, the tests themselves only assert the output was compatible with the output buildbot expected as of the some date. Lack of such English could lead to someone changing the output format AND the tests only to cause Buildbot breakage once a WebKit.org admin pushes a change to Builbot.

> If we are still interested in pursuing tests for the output then can we at least mock out the calls to JSC so that we do not need to have a dependency on it?

In that way, I would need a definition of the desirable mocking. The current patch has some mocking tests - as a small subset clones from Test262 tests. The tests for the buildbot mocks the output itself. I wonder if we could tackle this progressively, considering the density and all friction found here in this issue. The initial idea was to bring a starter for the unit tests, it seems we are going beyond now and this could only cause more friction.

> > > If the former, then I suggest we make more use of webkitdirs.pm as it will simplify the code and make it more consistent with our other Perl code. 
> 
> > I sent a patch for this already and it got rejected. We also considered it
> > would be a unwelcome burden having to tell everyone to keep consistency for
> > it with such an old version of Perl as it is not directly necessary for this
> > script.


> Do you have a bug # for this?

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

[valerie]

Created attachment 342196 [details]
Patch

[EWS Watchlist]

Comment on attachment 342196 [details]
Patch

Attachment 342196 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/8064093

New failing tests:
http/tests/security/canvas-remote-read-remote-video-blocked-no-crossorigin.html

[EWS Watchlist]

Created attachment 342205 [details]
Archive of layout-test-results from ews202 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews202  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Michael Saboff]

Comment on attachment 342196 [details]
Patch

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

Move the BSD license text into the source files that reference it, then delete the LICENSE file.  After that, I think you are good to go.

> Tools/Scripts/webkitperl/test262_unittest/fixtures/test/LICENSE:26
> +Copyright (C) 2018 Bocoup LLC. All rights reserved.
> +
> +Redistribution and use in source and binary forms, with or without
> +modification, are permitted provided that the following conditions
> +are met:
> +
> +1. Redistributions of source code must retain the above
> +   copyright notice, this list of conditions and the following
> +   disclaimer.
> +2. Redistributions in binary form must reproduce the above
> +   copyright notice, this list of conditions and the following
> +   disclaimer in the documentation and/or other materials
> +   provided with the distribution.
> +
> +THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDER "AS IS" AND ANY
> +EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> +IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
> +PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER BE
> +LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY,
> +OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
> +PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR
> +PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> +THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR
> +TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
> +THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> +SUCH DAMAGE.

Instead of referencing this file, include the license text directly at the top of each file.  Then delete this file.

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

Include the BSD license text directly here and in every other file where you reference the LICENSE file.

[valerie]

Created attachment 342543 [details]
Patch

[valerie]

Created attachment 342544 [details]
Patch

[valerie]

This bug depends on https://bugs.webkit.org/show_bug.cgi?id=186399

[Michael Saboff]

Comment on attachment 342544 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 342544 [details]
Patch

Clearing flags on attachment: 342544

Committed r232756: <https://trac.webkit.org/changeset/232756>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

(In reply to WebKit Commit Bot from comment #46)
> Comment on attachment 342544 [details]
> Patch
> 
> Clearing flags on attachment: 342544
> 
> Committed r232756: <https://trac.webkit.org/changeset/232756>

webkitperl-test is failing on the bots after this change:

Cannot find jsc, try with --release or specify with --jsc <path>.

Compilation failed in require at /Volumes/Data/slave/sierra-release-tests-wk2/build/Tools/Scripts/test262-runner line 45.
BEGIN failed--compilation aborted at /Volumes/Data/slave/sierra-release-tests-wk2/build/Tools/Scripts/test262-runner line 45.

#   Failed test 'expectations yaml file format'
#   at Tools/Scripts/webkitperl/test262_unittest/test262-runner-tests.pl line 156.
# Looks like you failed 1 test of 13.

https://build.webkit.org/builders/Apple%20Sierra%20Release%20WK2%20(Tests)/builds/9915/steps/webkitperl-test/logs/stdio

[valerie]

I just saw it! Patch coming in moments.

[valerie]

(In reply to Ryan Haddad from comment #48)
> (In reply to WebKit Commit Bot from comment #46)
> > Comment on attachment 342544 [details]
> > Patch
> > 
> > Clearing flags on attachment: 342544
> > 
> > Committed r232756: <https://trac.webkit.org/changeset/232756>
> 
> webkitperl-test is failing on the bots after this change:
> 
> Cannot find jsc, try with --release or specify with --jsc <path>.
> 
> Compilation failed in require at
> /Volumes/Data/slave/sierra-release-tests-wk2/build/Tools/Scripts/test262-
> runner line 45.
> BEGIN failed--compilation aborted at
> /Volumes/Data/slave/sierra-release-tests-wk2/build/Tools/Scripts/test262-
> runner line 45.
> 
> #   Failed test 'expectations yaml file format'
> #   at Tools/Scripts/webkitperl/test262_unittest/test262-runner-tests.pl
> line 156.
> # Looks like you failed 1 test of 13.
> 
> https://build.webkit.org/builders/Apple%20Sierra%20Release%20WK2%20(Tests)/
> builds/9915/steps/webkitperl-test/logs/stdio

Here is a bug for the fix: https://bugs.webkit.org/show_bug.cgi?id=186573