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

improve testing compatibility with Node.js #5204

Merged
merged 1 commit into from Dec 6, 2021

Conversation

alexlamsl
Copy link
Collaborator

No description provided.

@alexlamsl
Copy link
Collaborator Author

@kzc this should address #5202 (comment) − if you don't mind give it a spin on your OS since mine are all done on Windows 😉

@alexlamsl alexlamsl merged commit d2e7c4a into mishoo:master Dec 6, 2021
@alexlamsl alexlamsl deleted the compat branch December 6, 2021 14:42
@kzc
Copy link
Contributor

kzc commented Dec 6, 2021

With this PR:

$ node-v4.2.1 test/compress.js 
...
--- unicode.js
    Running test [ascii_only_false]
    Running test [ascii_only_true]
    Running test [unicode_parse_variables]
    Running test [unicode_escaped_identifier_1]
    Running test [unicode_escaped_identifier_2]
!!! Invalid input or expected stdout
---INPUT---
{
    var a = "foo";
    var 𐀀 = "bar";
    console.log(a, 𐀀);
}
---EXPECTED STDOUT---
foo bar

---ACTUAL ERROR---
SyntaxError: Unexpected token ILLEGAL


    Running test [unicode_identifier_ascii_only]
    Running test [unicode_string_literals]
    Running test [check_escape_style]
    Running test [escape_non_escaped_identifier]
    Running test [non_escape_2_non_escape]
    Running test [issue_2242_1]
    Running test [issue_2242_2]
    Running test [issue_2242_3]
    Running test [issue_2242_4]
    Running test [issue_2569]
    Running test [surrogate_pair]
!!! Invalid input or expected stdout
---INPUT---
{
    var 丽 = {
        "丸": "􀀀"
    };
    丽.乁 = "􀀁";
    console.log(typeof 丽, 丽.丸, 丽["乁"]);
}
---EXPECTED STDOUT---
object 􀀀 􀀁

---ACTUAL ERROR---
SyntaxError: Unexpected token ILLEGAL


    Running test [surrogate_pair_ascii]
!!! Invalid input or expected stdout
---INPUT---
{
    var 丽 = {
        "丸": "􀀀"
    };
    丽.乁 = "􀀁";
    console.log(typeof 丽, 丽.丸, 丽["乁"]);
}
---EXPECTED STDOUT---
object 􀀀 􀀁

---ACTUAL ERROR---
SyntaxError: Unexpected token ILLEGAL
...
!!! Failed 3 test case(s).
!!! unicode.js
$ node-v6.9.0 test/compress.js
$  # 0 failures
$ node-v8.0.0 test/compress.js 
...
    Running test [issue_4974]
!!! failed
---INPUT---
{
    (async function f() {
        return 42 in f();
    })();
    console.log("PASS");
}
---EXPECTED ERROR---
Error: node-v8.0.0[96706]: ../src/async-wrap.cc:621:Local<v8::Value> node::AsyncWrap::MakeCallback(const Local<v8::Function>, int, Local<v8::Value> *): Assertion `(env()->current_async_id()) == (0)' failed.
...
!!! Failed 1 test case(s).
!!! awaits.js
$ node-v10.4.1 test/compress.js 
$  # 0 failures
$ node-v12.13.0 test/compress.js 
$  # 0 failures
$ node-v14.3.0 test/compress.js 
$  # 0 failures

@alexlamsl
Copy link
Collaborator Author

alexlamsl commented Dec 6, 2021

$ node-v4.2.1 test/compress.js

So Unicode parsing is broken on macOS somehow? 😓

$ node-v8.0.0 test/compress.js

Given the fact that input & expect are identical − can you try and see if this test failure is always repeatable?

(you can use node-v8.0.0 test/compress.js awaits.js to run that single file)

Oh wait I know what might have happened − you may have cropped the ACTUAL ERROR line just after, but looking at the EXPECTED ERROR your message contains what looks like the process ID ([96706]), which will be difference between runs... 😅

@alexlamsl
Copy link
Collaborator Author

Okay this is starting to feel creepy...

$ uname -a
Darwin MBP.local 15.6.0 Darwin Kernel Version 15.6.0: Thu Jun 21 20:07:40 PDT 2018; root:xnu-3248.73.11~1/RELEASE_X86_64 x86_64
$ node -v
v4.2.1
$ node test/compress unicode.js
--- unicode.js
    Running test [ascii_only_false]
    Running test [ascii_only_true]
    Running test [unicode_parse_variables]
    Running test [unicode_escaped_identifier_1]
    Running test [unicode_escaped_identifier_2]
    Running test [unicode_identifier_ascii_only]
    Running test [unicode_string_literals]
    Running test [check_escape_style]
    Running test [escape_non_escaped_identifier]
    Running test [non_escape_2_non_escape]
    Running test [issue_2242_1]
    Running test [issue_2242_2]
    Running test [issue_2242_3]
    Running test [issue_2242_4]
    Running test [issue_2569]
    Running test [surrogate_pair]
    Running test [surrogate_pair_ascii]

i.e. can't reproduce those SyntaxErrors with the same Node.js version & OS 😕

@alexlamsl
Copy link
Collaborator Author

With the following patch:

--- a/test/sandbox.js
+++ b/test/sandbox.js
@@ -248,16 +248,23 @@ function run_code_vm(code, toplevel, timeout) {
     process.stdout.write = function(chunk) {
         stdout += chunk;
     };
+console.error('====RUN====');
+console.error(code);
+console.error('-----------');
     try {
         var ctx = vm.createContext({ console: console });
         // for Node.js v6
         vm.runInContext(setup_code, ctx);
         vm.runInContext(toplevel ? "(function(){" + code + "})();" : code, ctx, { timeout: timeout });
         // for Node.js v4
+console.error('DONE', stdout);
+console.error('===========');
         return strip_color_codes(stdout.replace(/\b(Array \[|Object {)/g, function(match) {
             return match.slice(-1);
         }));
     } catch (ex) {
+console.error('THROWN', ex);
+console.error('====!!!====');
         return ex;
     } finally {
         process.stdout.write = original_write;

Running test/compress unicode.js gives:

...
    Running test [unicode_escaped_identifier_2]
====RUN====
var a="foo";var 𐀀="bar";console.log(a,𐀀);
-----------
DONE foo bar

===========
====RUN====
var a="foo";var 𐀀="bar";console.log(a,𐀀);
-----------
DONE foo bar

===========
...

In my case running identical inputs twice through sandbox.run_code().

@alexlamsl
Copy link
Collaborator Author

In all of test/compress, those three test cases are the only ones with non-ASCII characters excceding code point 0xFFFF, so that's probably the culprit.

Why it only happens on your copy of macOS though remains a mystery 😓

@kzc
Copy link
Contributor

kzc commented Dec 7, 2021

$ node-v4.2.1 test/compress.js
!!! Failed 3 test case(s).
!!! unicode.js
So Unicode parsing is broken on macOS somehow? 😓

I'm running an old version of macos, Darwin 13.4.0 x86_64, so that could be the problem. But then again, all unicode tests run fine on node v6.x through v16.x. Maybe that version of NodeJS relied on a system library and the subsequent releases had unicode built in?

$ node-v8.0.0 test/compress.js
Given the fact that input & expect are identical − can you try and see if this test failure is always repeatable?
(you can use node-v8.0.0 test/compress.js awaits.js to run that single file)

node-v8.0.0 hard crashes on issue_4974 every single time on my machine.

But now that I look at that test, it infinitely recurses:

$ echo '{(async function f(){return 42 in f()})();console.log("PASS")}' | node-v16.1.0 
Exception in PromiseRejectCallback:
[stdin]:1
{(async function f(){return 42 in f()})();console.log("PASS")}
                                     ^
RangeError: Maximum call stack size exceeded
PASS

Granted, it shouldn't outright crash out of node-v8.0.0, but it does politely exhaust the stack in subsequent major Node releases. Was that the intention of the test?

In any case, I wouldn't lose sleep over these failing tests on old node versions. You could bump the versions of those tests to the next major to avoid the flakes on my machine if you want.

@alexlamsl
Copy link
Collaborator Author

So the only remaining issue is with node-v4.2.1 − here I thought I was an old version of macOS 😏

I do recall Node.js team were quibbling about ICU back in the days, so it's likely that they didn't statically linked to one that provides the appropriate Unicode version and just rely on whatever's available.

Problem is I can't reproduce this locally, so even if I were to workaround this by bumping node_version, I have no idea which point release would be the safe bet.

@kzc
Copy link
Contributor

kzc commented Dec 7, 2021

Node v4.x was not a great series. It's admirable that you're still supporting it, but I wonder if anyone is using it.

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

Successfully merging this pull request may close these issues.

None yet

2 participants