Hook up js-interpreter-tyrant test runner for our patched interpreter #13974

pcardune · 2017-03-23T21:34:48Z

Now we can patch it like crazy and have an idea of what we broke and/or fixed!

…cardune-interpreter-tests

islemaster

I've got a couple of questions but this is all looking really good.

islemaster · 2017-03-24T21:38:49Z

apps/package.json

@@ -14,6 +14,7 @@
    "test:unit": "grunt unitTest",
    "test:integration": "grunt integrationTest",
    "test:entry": "grunt karma:entry",
+    "test:interpreter": "./node_modules/@code-dot-org/js-interpreter-tyrant/bin/run.js --run --diff --root ./test/interpreter --interpreter ./test/interpreter/patched-interpreter.js --progress",


Because you list "bin": "bin/run.js" in js-interpreter-tyrant's package.json, it will be symlinked into node_modules/.bin, which is already in the path when running an npm script. That means (I think) you can write this line as:

"test:interpreter": "run.js --run --diff --root ./test/interpreter --interpreter ./test/interpreter/patched-interpreter.js --progress",

...and probably should, as it's a little less configuration-dependent (it's possible customize the node_modules and .bin locations, though I'm not sure why you would).

This would be even nicer if you had js-interpreter-tyrant export a js-interpreter-tyrant executable instead, so it could be

"test:interpreter": "js-interpreter-tyrant --run --diff --root ./test/interpreter --interpreter ./test/interpreter/patched-interpreter.js --progress",

...as you describe in the README for that tool.

The issue here is yarn 0.16.1. It doesn't actually put the js-interpreter-tyrant script in node_modules/.bin because js-interpreter-tyrant is under the @code-dot-org scope :(. So it ends up in node_modules/@code-dot-org/.bin which doesn't make any sense and isn't in the path. This has been fixed in newer versions of yarn. So we really need to upgrade.

Ugh, what a headache. Leave it for now. Hopefully we'll remember to clean it up after we upgrade.

islemaster · 2017-03-24T21:42:37Z

apps/package.json

@@ -34,10 +35,15 @@
  "browser": {
    "blockly": "./build/package/js/blockly.js"
  },
+  "resolutions": {
+    "eshost": "github:pcardune/eshost#f3fa7fa",
+    "test262-compiler": "github:pcardune/test262-compiler#ffa0dff"


Two questions:

It looks like the resolutions field is a yarn-specific extension of package.json. Are we no longer backwards-compatible with npm?

What is customized about these repos, and do we expect to switch back to an upstream at some point?

I think I might be able to get rid of this resolutions thing actually. It should get taken care of by what is in the yarn.lock file.

eshost includes a new agent definition for js-interpreter. I have an issue out asking for a better way to add new agents without having them live in the eshost repo. Haven't heard back yet :(. test262-compiler just has a small (but important) bug fix which I likewise have a pull request out for but haven't heard back yet either.

pull requests in question:

Don't crash when unable to parse error props bterlson/eshost#12

Use sta.js helper instead of defining custom $ERROR and Test262Error bterlson/test262-compiler#1

Awesome 👍

islemaster · 2017-03-24T21:44:27Z

apps/src/lib/tools/jsinterpreter/patchInterpreter.js

+      scope = scope.parentScope;
+    }
+    this.throwException('Unknown identifier: ' + nameStr);
+    return this.UNDEFINED;


Curious coverage report here. Does this.throwException throw a native exception? If so, should the return line be removed as unreachable?

It throws an exception sometimes. The case where it doesn't is definitely not covered here!

Though frankly, this code is only really covered by integration tests, so I really need to go back and add unit tests.

islemaster · 2017-03-24T21:46:28Z

apps/test/interpreter/test-on-circle.sh

@@ -0,0 +1,6 @@
+#!/bin/bash
+set -x
+./node_modules/@code-dot-org/js-interpreter-tyrant/bin/run.js --threads 1 --run --diff --verbose


Similar note here, but since node_modules/.bin isn't in scope you'd use

`npm bin`/run.js --threads 1 --run --diff --verbose

If we're dropping npm support, I suppose `yarn bin` is preferable - it works the same way.

same issue with yarn 0.16.1 as stated above.

islemaster · 2017-03-24T21:48:20Z

lib/rake/test.rake

+    task :interpreter do
+      run_tests_if_changed('interpreter', ['apps/src/lib/tools/jsinterpreter/patchInterpreter.js']) do
+        TestRunUtils.run_interpreter_tests
+      end


So... how long does this take, on one container on Circle?

a couple hours I think? Haven't gotten it to run yet so will find out soon!

Well, I guess it takes more than 2 hours because circle is timing out. I don't think there is a particularly quick fix for that.

pcardune · 2017-03-23T21:36:01Z

apps/src/lib/tools/jsinterpreter/patchInterpreter.js

@@ -0,0 +1,99 @@
+const Interpreter = require('@code-dot-org/js-interpreter');
+
+module.exports = function patchInterpreter() {


This is all just extracted from JSInterpreter.js. We have to use node.js style exports because this needs to run inside node and the browser.

pcardune · 2017-03-23T21:36:54Z

tools/hooks/lint.rb

@@ -19,7 +19,7 @@ def filter_rubocop(modified_files)
    RUBY_EXTENSIONS.any? {|ext| f.end_with? ext }
  end
  modified_ruby_scripts = modified_files.select do |f|
-    first_line = File.exist?(f) ? File.open(f).first : nil
+    first_line = File.file?(f) ? File.open(f).first : nil


needed to change this so it wouldn't get screwed up with git submodules which show up in git's list of "modified files" but is actually a directory.

pcardune · 2017-03-27T16:04:32Z

apps/package.json

@@ -14,6 +14,7 @@
    "test:unit": "grunt unitTest",
    "test:integration": "grunt integrationTest",
    "test:entry": "grunt karma:entry",
+    "test:interpreter": "./node_modules/@code-dot-org/js-interpreter-tyrant/bin/run.js --run --diff --root ./test/interpreter --interpreter ./test/interpreter/patched-interpreter.js --progress",


The issue here is yarn 0.16.1. It doesn't actually put the js-interpreter-tyrant script in node_modules/.bin because js-interpreter-tyrant is under the @code-dot-org scope :(. So it ends up in node_modules/@code-dot-org/.bin which doesn't make any sense and isn't in the path. This has been fixed in newer versions of yarn. So we really need to upgrade.

pcardune · 2017-03-27T16:07:38Z

apps/package.json

@@ -34,10 +35,15 @@
  "browser": {
    "blockly": "./build/package/js/blockly.js"
  },
+  "resolutions": {
+    "eshost": "github:pcardune/eshost#f3fa7fa",
+    "test262-compiler": "github:pcardune/test262-compiler#ffa0dff"


I think I might be able to get rid of this resolutions thing actually. It should get taken care of by what is in the yarn.lock file.

eshost includes a new agent definition for js-interpreter. I have an issue out asking for a better way to add new agents without having them live in the eshost repo. Haven't heard back yet :(. test262-compiler just has a small (but important) bug fix which I likewise have a pull request out for but haven't heard back yet either.

pull requests in question:

Don't crash when unable to parse error props bterlson/eshost#12

Use sta.js helper instead of defining custom $ERROR and Test262Error bterlson/test262-compiler#1

pcardune · 2017-03-27T16:09:55Z

apps/src/lib/tools/jsinterpreter/patchInterpreter.js

+      scope = scope.parentScope;
+    }
+    this.throwException('Unknown identifier: ' + nameStr);
+    return this.UNDEFINED;


It throws an exception sometimes. The case where it doesn't is definitely not covered here!

Though frankly, this code is only really covered by integration tests, so I really need to go back and add unit tests.

pcardune · 2017-03-27T16:10:18Z

apps/test/interpreter/test-on-circle.sh

@@ -0,0 +1,6 @@
+#!/bin/bash
+set -x
+./node_modules/@code-dot-org/js-interpreter-tyrant/bin/run.js --threads 1 --run --diff --verbose


same issue with yarn 0.16.1 as stated above.

pcardune · 2017-03-27T16:10:48Z

lib/rake/test.rake

+    task :interpreter do
+      run_tests_if_changed('interpreter', ['apps/src/lib/tools/jsinterpreter/patchInterpreter.js']) do
+        TestRunUtils.run_interpreter_tests
+      end


a couple hours I think? Haven't gotten it to run yet so will find out soon!

islemaster

Changes LGTM (and I'm very excited about having this fantastic coverage integrated). Looks like some pretty gnarly failures in the Circle build though. Any idea what's going on there?

pcardune · 2017-03-27T18:41:39Z

Broken tests here are due to a bug I introduced in the most recent release of @code-dot-org/js-interpreter. Fixing that here first: code-dot-org/JS-Interpreter#5

…o fix test failures

…cardune-interpreter-tests

pcardune added 5 commits March 23, 2017 14:13

Hook up js-interpreter-tyrant to our patched interpreter

35df63a

Merge branch 'staging' of github.com:code-dot-org/code-dot-org into p…

46040fa

…cardune-interpreter-tests

Automatically run interpreter tests when we change our patches

b8d1bd0

Small fixups

f4cc0ad

add current test results for future comparison

5694eaa

pcardune requested a review from islemaster March 23, 2017 21:37

pcardune added 2 commits March 23, 2017 22:31

Merge branch 'staging' of github.com:code-dot-org/code-dot-org into p…

a6be058

…cardune-interpreter-tests

Merge branch 'staging' of github.com:code-dot-org/code-dot-org into p…

a1b812b

…cardune-interpreter-tests

islemaster reviewed Mar 24, 2017

View reviewed changes

pcardune commented Mar 27, 2017

View reviewed changes

islemaster approved these changes Mar 27, 2017

View reviewed changes

pcardune added 5 commits March 28, 2017 00:09

Update to newer version of js-interpreter and js-interpreter-tyrant t…

837df56

…o fix test failures

Merge branch 'staging' of github.com:code-dot-org/code-dot-org into p…

e5e4a1f

…cardune-interpreter-tests

fix running of test262 suite on circle

a53f1d8

I guess circle does not like git submodules for some reason

7ae67ef

disable interpreter tests on circle for now cause they take too long

d89fd6a

pcardune merged commit 20e9c33 into staging Mar 28, 2017

pcardune deleted the pcardune-interpreter-tests branch March 28, 2017 22:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hook up js-interpreter-tyrant test runner for our patched interpreter #13974

Hook up js-interpreter-tyrant test runner for our patched interpreter #13974

pcardune commented Mar 23, 2017

islemaster left a comment

islemaster Mar 24, 2017

pcardune Mar 27, 2017

islemaster Mar 27, 2017

islemaster Mar 24, 2017

pcardune Mar 27, 2017

islemaster Mar 27, 2017

islemaster Mar 24, 2017

pcardune Mar 27, 2017

islemaster Mar 24, 2017

pcardune Mar 27, 2017

islemaster Mar 24, 2017

pcardune Mar 27, 2017

pcardune Mar 28, 2017

pcardune Mar 23, 2017

pcardune Mar 23, 2017

pcardune Mar 27, 2017

pcardune Mar 27, 2017

pcardune Mar 27, 2017

pcardune Mar 27, 2017

pcardune Mar 27, 2017

islemaster left a comment

pcardune commented Mar 27, 2017

		@@ -0,0 +1,99 @@
		const Interpreter = require('@code-dot-org/js-interpreter');

		module.exports = function patchInterpreter() {

Hook up js-interpreter-tyrant test runner for our patched interpreter #13974

Hook up js-interpreter-tyrant test runner for our patched interpreter #13974

Conversation

pcardune commented Mar 23, 2017

islemaster 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

islemaster left a comment

Choose a reason for hiding this comment

pcardune commented Mar 27, 2017