Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Script that creates web-platform-test wrappers for test262's tests #8980

Closed
wants to merge 28 commits into from

Conversation

dpino
Copy link
Contributor

@dpino dpino commented Jan 10, 2018

The scripts reads out 'test262/test' and generates web-platform-test wrappers for each test. The generated tests are stored at 'js/test262'.

Each generated web-platform-test runs the target test262 test in 4 different agents: IFrame, Window, DedicatedWorker and SharedWorker. For each agent usually two tests are generated: one in strict-mode and another one in non-strict mode (unless the test indicates otherwise).

For IFrame and Window agents an HTML file is created on-the-fly containing the target test. In the case of DedicatedWorker and SharedWorker a JavaScript test is created on-the-fly containing the target test. The harnessing code runs the test and checks whether there was any error.

Currently when running the generated web-platform-test wrappers many tests fail. More details at issue #8308

I post this PR with no intention of merging it yet, but to gather feedback and discuss what needs to be done from here to get the PR landed.


This change is Reviewable

The generated tests run the target test262 within different agent.
Currently supported agents are: IFrame, Window, DedicatedWorker and
SharedWorker. For each agent two tests are generated, one in strict mode
and another in non-strict mode, unless the test indicates otherwise.

The generated tests are placed at js/test262/.
@dpino
Copy link
Contributor Author

dpino commented Jan 10, 2018

To generate the tests target dir should point to root folder of test262 (https://github.com/tc39/test262).

$ ./test262-to-wpt test262
Generating web-platform-test wrappers for test262
Output: js/test262

Now run one of the generated WPT tests, for instance:

$ ./wpt run firefox js/test262/intl402/Date/prototype/toLocaleDateString/13.3.2_L15.html
web-platform-test
~~~~~~~~~~~~~~~~~
Ran 9 checks (1 tests, 8 subtests)
Expected results: 9
OK

Helper script to run all the tests and save output to file. Since several tests fail the tests are run with no-restart-on-unexpected flag on, to speed up overall running time.

run-all.sh

#!/usr/bin/env bash

FOLDERS=(
   annexB
   built-ins
   harness
   intl402
   language
)

for each in ${FOLDERS[@]}; do
   time ./wpt run --no-restart-on-unexpected firefox "js/test262/test/${each}" | tee "${each}.output"
done

@dpino
Copy link
Contributor Author

dpino commented Jan 10, 2018

Some considerations regarding the PR:

  • The generated web-platform-tests depend on test262's harness. Test262's harness code is expected to be found at folder 'resources/test262/harness'. This code is not part of the PR. Perhaps it should be copied when the tests are generated.
  • Should the generated tests be part of web-platform-test codebase? If so it would be necessary to discuss an updating process. Perhaps by maintaining a file with a timestamp for the original tests and generated test. Ideally generated test should not be manually modified.
  • Supporting ServiceWorkers is tricky as they cannot be generate on-the-fly with a Blob. Probably support can be added if the ServiceWorker test is generated beforehand along with the WPT generation process.
  • 262 tests that use the JavaScript Shell Agent API should be treated in a special manner, covering all the possible scenarios for the agents supported (See Generalize tests that use the agent abstraction tc39/test262#1351).

@ghost
Copy link

ghost commented Jan 10, 2018

Build PASSED

Started: 2018-01-28 21:43:40
Finished: 2018-01-28 21:59:45

View more information about this build on:

return global.$262;
}

// TODO: $DONE is used by Promise tests and others, but I don't know from where to fetch it.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$DONE is added by test262-compiler: https://github.com/bterlson/test262-compiler/blob/master/lib/compilerdeps.js, but the plan is to eliminate the test262-compiler dependency by consolidating the "compiler" operations and boilerplate into test262-stream (bocoup/test262-stream#7). Once that's complete, test262-harness is going to be updated to use test262-stream.

@rwaldron
Copy link
Contributor

I recommend blacklisting the test262/test/harness directory, this is just tests for test262's harness itself and is of no value outside of test262.

test = "'use strict';\n" + test;
}
if (attrs.type == 'SyntaxError' && attrs.phase == 'runtime') {
// If phase is 'runtime' the error will be caught here. If phase is 'early' the
Copy link
Contributor

@rwaldron rwaldron Jan 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

negative: phases are more specific in the latest version (2.0.0) of https://github.com/tc39/test262/blob/master/INTERPRETING.md, see negative for more details, but the short version is that early is now parse.

You can safely check for both for the time being.

// error will be handled at the window.onerror event.
output.push('try { eval("');
output.push(escapeEvalExpr(test));
output.push('"); }\ncatch (e) { assert.sameValue(e instanceof SyntaxError, true); }');
Copy link
Contributor

@rwaldron rwaldron Jan 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@rwaldron rwaldron Jan 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's a better illustration from firefox nightly:

This is code exactly as prepareTest(...) returns it:

try { eval("import x from './instn-named-err-not-found-empty_FIXTURE.js';"); }
catch (e) { assert.sameValue(e instanceof SyntaxError, true); }

If I modify it to:

try { eval("import x from './instn-named-err-not-found-empty_FIXTURE.js';"); }
catch (e) { console.log(e); }

And put it in an html file, with the same <script type="text/javascript"> (this is important) :

<!DOCTYPE html>
<script type="text/javascript">
  try { eval("import x from './instn-named-err-not-found-empty_FIXTURE.js';"); }
  catch (e) { console.log(e); }
</script>

This will log:

SyntaxError: import declarations may only appear at top level of a module

Which results in a false positive for this test.

If I change that to (<script type="module"> is the change):

<!DOCTYPE html>
<script type="module">
  try { eval("import x from './instn-named-err-not-found-empty_FIXTURE.js';"); }
  catch (e) { console.log(e); }
</script>

...The same error occurs. Also, in neither case does a network request get made for instn-named-err-not-found-empty_FIXTURE.js.

Finally, when I run this:

<!DOCTYPE html>
<script type="module">
import x from './instn-named-err-not-found-empty_FIXTURE.js';
</script>

The outcome is:

SyntaxError: import not found: default

Which is the SyntaxError this test was written to verify. There is also a network request made for instn-named-err-not-found-empty_FIXTURE.js. (I did this whole exercise in Firefox Nightly with dom.moduleScripts.enabled set to true and Safari Technology Preview.


Note: I had to manually make a file called instn-named-err-not-found-empty_FIXTURE.js, because the real instn-named-err-not-found-empty_FIXTURE.js has been turned into a file called instn-named-err-not-found-empty_FIXTURE.html, with all of the same treatments. Easily fixed by blacklisting _FIXTURE.js files.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch! I will rework the code as suggested.


<!-- Test262 required libraries -->
<script src="http://localhost:8000/resources/test262/harness/assert.js"><\/script>
<script src="http://localhost:8000/resources/test262/harness/sta.js"><\/script>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This domain doesn't work if the tests are run from http://web-platform.test:8000 (in fact, my localhost:8000 is not the same as http://web-platform.test:8000)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add .sub. to the filename and then you can use substitutions like {{host}} c.f. the rather outdated documentation at https://wptserve.readthedocs.io/en/latest/pipes.html#sub

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(or use just paths, of course, in this case, but in general the feature is there if you need it).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jgraham to clarify, do you mean, eg:

<script src="http://{{host}}:{{ports[http][0]}}/resources/test262/harness/sta.js"><\/script>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the hint. I wasn't happy with hardcoding the host and the port but I couldn't find other way to make it work. I'll fix.

test262-to-wpt Outdated
<head>
<meta charset="utf-8">
<title>###TITLE###</title>
<meta name="help" href="https://storage.spec.whatwg.org/#dom-storagemanager-persisted">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you meant to include this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, likely a copy-paste error.

if (typeof postMessage !== 'function') {
throw new Error('No method available to detach an ArrayBuffer');
} else {
postMessage(null, '*', [buffer]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strange indentation 0_o

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems there's a mix up of tabs and spaces in that file. I will review all the files and make sure there are only spaces.

@rwaldron
Copy link
Contributor

I know that @jugglinmike and @jgraham have discussed this extensively in the past and I want to make sure that all of their interests/concerns are heard and/or addressed.

dpino added 4 commits January 12, 2018 13:20
If a test expects a SyntaxError, regardless of the value of phase, the
error is always managed in window.onerror.
dpino added 17 commits January 15, 2018 11:45
Replace CR + LF for LF only.
Replace CR for LF.
Tests that use ES functions such as createRealm, evalScript or
dettachArrayBuffer are not supported in a Worker context.

These operations are implemented in a Window context by appending new
elements to the DOM (an iframe in the case of createRealm and a script
element in the case of evalScript). Workers don't have access to the
Window object. On the other hand, creating a new realm by instantiating
a new Worker cannot be a solution. The new realm (a Worker) won't
provide the same functions as a Window, which what the tests expect to find
after calling createRealm (for instance $262.createRealm().eval(...))
Tests marked with flag 'async' should include doneprintHandle.js.
Implement ES Shell function 'print' that receives a message
'Test262:AsyncTestComplete' in case the test completed successfully.
Otherwise, throw error. If print is never called, rely on WPT timeout
mechanism.
Tests with a negative property are supposed to throw an Exception. The
thrown exception should have the same type as negative.type.
Syntax errors, the type that are emitted during parse or early phase,
cab be caught if the test is wrapped within a try/catch.
For many tests Firefox throws a SyntaxError instead of a ReferenceError.
Many module-code tests fail cause they try to import or export a JavaScript
module which path is no longer valid. It's necesary to replace the path with
an URL. This change implies to activate CORS in the WPT WebServer.
@dpino
Copy link
Contributor Author

dpino commented Jan 28, 2018

I have pushed some more commits that help fixing several tests.

The most important change it's fixing tests that are related with modules. The tests complain they could not locate the module, since the path was broken. The following error was returned:

SyntaxError: import declarations may only appear at top level of a module

I tried to fix the path by using relative and absolute paths but it only worked if I replaced the original path with an URL. This led me to activate CORS in the WPT WebServer plus hardcoding "host" and "port" in the URL (which I know it's wrong). I tried to encode the URL using placeholders and passing ?pipe=sub to the URL that points to the target file containing the test, but it didn't work in this case (does this mechanism work with modules import too?).

@foolip
Copy link
Member

foolip commented Oct 20, 2019

@dpino what is the status of this? I see this has been stale for almost two years, but if you do want to keep working on it, I think it'd be appropriate to put it through the RFC process at https://github.com/web-platform-tests/rfcs.

There are also other external test suites that might be interesting to run as part of wpt, or at least get results for into wpt.fyi, see #19438 for a suggested experiment for Gecko's mochitests.

@annevk
Copy link
Member

annevk commented Oct 21, 2019

This was an experiment funded by Mozilla to see if there's value in running JavaScript tests with different types of agents. We didn't proceed eventually as it seems there doesn't appear to be a lot of value to be gained and properly abstracting everything would be quite a bit of effort.

@foolip
Copy link
Member

foolip commented Oct 21, 2019

I see, in that case I guess this PR should be closed?

@annevk
Copy link
Member

annevk commented Oct 21, 2019

I think that's reasonable, but I'll let @dpino make the call.

@foolip
Copy link
Member

foolip commented Oct 28, 2019

No reply from @dpino, so I'm closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants