Support running tests in inferior JavaScript repl. #1103

simonhj · 2017-10-27T00:27:38Z

Define two new flags for test-runner.js:
--repl : If present the, the prepack sources of test are evaluated
using by piping to the process instead of in a new node context.
--es5 : run babel on tests, used if the repl does not support es6.

To further deal with es6 a new test header flag is introduced, es6. If a test is
not relevant in a non es6 world, the test is skipped if the es5 flag is also set.
This flag has been added to some tests.

Running subprocess for each test is obviously more expensive so the default
behavior is unchanged and still uses a new context.

Define two new flags for test-runner.js: --repl <path> : If present the, the prepack sources of test are evaluated using by piping to the process instead of in a new node context. --es5 : run babel on tests, used if the repl does not support es6. To further deal with es6 a new test header flag is introduced, es6. If a test is not relevant in a non es6 world, the test is skipped if the es5 flag is set. This flag has been added to some tests. Running subprocess for each test is obviously more expensive so the default behavior is unchanged.

jeffreytan81

@simonhj, when es5 mode is used and tests are lowered to es5 code, why do we still want to skip es6 tests?

jeffreytan81 · 2017-10-27T18:40:49Z

scripts/test-runner.js

@@ -344,10 +427,14 @@ class ProgramArgs {
  debugNames: boolean;
  verbose: boolean;
  filter: string;
-  constructor(debugNames: boolean, verbose: boolean, filter: string) {
+  repl: string;


Can you add comments for the meaning of these two flags?

jeffreytan81 · 2017-10-27T18:42:34Z

scripts/test-runner.js

@@ -388,12 +475,14 @@ class ArgsParseError {
 // Parses through the command line arguments and throws errors if usage is incorrect
 function argsParse(): ProgramArgs {
  let parsedArgs = minimist(process.argv.slice(2), {
-    string: ["filter"],
-    boolean: ["debugNames", "verbose"],
+    string: ["filter", "repl"],


I do not feel "repl" is a very informative name regarding what it does. How about call it "outOfProcessRuntime"?

jeffreytan81 · 2017-10-27T18:51:31Z

scripts/test-runner.js


  for (let test of tests) {
    // filter hidden files
    if (path.basename(test.name)[0] === ".") continue;
    if (test.name.endsWith("~")) continue;
    if (test.file.includes("// skip")) continue;
+    if (args.es5 && test.file.includes("// es6")) continue;


Are you excluding es6 tests from running under es5 mode? I thought you wanted to babel lower es6 tests to es5 code for running?

Yes. I am not sure what the right thing to do here is. I added the babel lowering to deal with test that use ES6 features like let and for-of loops, such they could still run in an es5 context. However some tests explicitly test ES6 features, such as symbols for intstance and testing them in es5 mode is pointless. Futhermore babel won't fix a test using the Symbol API for example.
So I ended up having both, but I am open to ideas.

I see.
In this situation, it seems wasteful to babel lower for non-es6 tests under es5 mode.
Or if you still want to opt-in some es6 tests for es5 mode, you can introduce two flags: es6-can-lower, es6-cannot-lower(you may change the wording).

Why is it wasteful? Seems simpler to me, if you are dealing with an es5-only runtime and the majority of tests was not written with this mindset, lowering them to get rid of syntactical/semantics features seems the cleanest approach allowing you to still use the test. I only added the flag to deal with tests that can't be lowered properly. Would "es6-only" be a better name?

The fact that, 90% of the tests are es5-only tests which do not need to be babel lowered; they are all transformed by additional babel lowering API call, is the thing I feel wasteful.
We can avoid this by introducing three categories: default(no babel call), es6-can-lower(call babel on it), es6-cannot-lower(skip for es5 mode).
But I am fine if you insist on your design since it only slows down test-runner.

its a fair point. But your design requires me to go through all 700 or so tests and categorize all of them, which I would like to avoid :).

Hmm, I thought you just need to mark es6 tests and decide which can and can't be lowered?
If we want to do it, here are the logic steps in my mind:

Directly run the es5 runtime for all the tests. Any failed tests are es6 tests(which I hope are not many). Any succeeded tests are es5 tests do not need babel lowering.

Mark these es6 tests with es6-can-lower flag which tells test-runner to babel lower them and run them. Any failed tests are es6 tests that can't be lowered(mark them with es6-cannot-lower flag).

Done.

Yes. I am not saying it can't be done. I am just saying I think its a lot of work just to avoid some babel invocations.
If you feel strongly about it, I can do it. But it seems like a lot, for what we are trying achieve here.

Yeah, I am fine with this.

jeffreytan81 · 2017-10-27T18:53:31Z

scripts/test-runner.js

        } catch (e) {
-          actual = e;
+          // always compare strings.
+          actual = "" + e;


Is e.toString() more readable?

Its a pretty common JS idiom to force string conversion (and actually faster than toString !)

- better naming and doc - linting

jeffreytan81 · 2017-10-30T17:11:13Z

scripts/test-runner.js

@@ -314,16 +371,41 @@ function runTest(name, code, options, args) {
    return false;
  }
 }
+
+function testInferiorRepl(replPath) {


Change this function to "testInOutOfProcessRuntime(runtimePath)" to align with the option name change.

Also, the name is a bit confusing to me. Based on its implementation, prepareReplExternalSepc() indicates its purpose better.

jeffreytan81 · 2017-10-30T17:16:06Z

scripts/test-runner.js


  for (let test of tests) {
    // filter hidden files
    if (path.basename(test.name)[0] === ".") continue;
    if (test.name.endsWith("~")) continue;
    if (test.file.includes("// skip")) continue;
+    if (args.es5 && test.file.includes("// es6")) continue;


I see.
In this situation, it seems wasteful to babel lower for non-es6 tests under es5 mode.
Or if you still want to opt-in some es6 tests for es5 mode, you can introduce two flags: es6-can-lower, es6-cannot-lower(you may change the wording).

jeffreytan81 · 2017-10-30T17:17:06Z

scripts/test-runner.js

@@ -257,10 +308,16 @@ function runTest(name, code, options, args) {
          }
        }
        if (markersIssue) break;
+        let codeToRun = addedCode + newCode;
+        if (args.es5) {


Sounds like there is no need to lower using babel under es5 mode since we exclude es6 tests from running under es5 mode.

facebook-github-bot

@simonhj is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Summary: Define two new flags for test-runner.js: --repl <path> : If present the, the prepack sources of test are evaluated using by piping to the process instead of in a new node context. --es5 : run babel on tests, used if the repl does not support es6. To further deal with es6 a new test header flag is introduced, es6. If a test is not relevant in a non es6 world, the test is skipped if the es5 flag is also set. This flag has been added to some tests. Running subprocess for each test is obviously more expensive so the default behavior is unchanged and still uses a new context. Closes facebookarchive#1103 Differential Revision: D6192670 Pulled By: simonhj fbshipit-source-id: 35e4b819d863634f3273a4da7984c7a71d8c4fb9

facebook-github-bot added the CLA Signed label Oct 27, 2017

jeffreytan81 reviewed Oct 27, 2017

View reviewed changes

Address review comments:

3479572

- better naming and doc - linting

jeffreytan81 reviewed Oct 30, 2017

View reviewed changes

jeffreytan81 approved these changes Oct 30, 2017

View reviewed changes

Better naming

c37db93

facebook-github-bot reviewed Oct 30, 2017

View reviewed changes

facebook-github-bot closed this in 0a3e23e Oct 31, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support running tests in inferior JavaScript repl. #1103

Support running tests in inferior JavaScript repl. #1103

simonhj commented Oct 27, 2017

jeffreytan81 left a comment

jeffreytan81 Oct 27, 2017

simonhj Oct 27, 2017

jeffreytan81 Oct 27, 2017

jeffreytan81 Oct 27, 2017

simonhj Oct 27, 2017 •

edited

Loading

jeffreytan81 Oct 30, 2017

simonhj Oct 30, 2017

jeffreytan81 Oct 30, 2017

simonhj Oct 30, 2017

jeffreytan81 Oct 30, 2017

simonhj Oct 30, 2017

jeffreytan81 Oct 30, 2017

jeffreytan81 Oct 27, 2017

simonhj Oct 27, 2017

jeffreytan81 Oct 30, 2017

jeffreytan81 Oct 30, 2017

jeffreytan81 Oct 30, 2017

jeffreytan81 Oct 30, 2017

facebook-github-bot left a comment

Support running tests in inferior JavaScript repl. #1103

Support running tests in inferior JavaScript repl. #1103

Conversation

simonhj commented Oct 27, 2017

jeffreytan81 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonhj Oct 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

simonhj Oct 27, 2017 •

edited

Loading