Make tests engine agnostic #16272

Trott · 2017-10-18T00:05:54Z

We treat error message changes as breaking changes and therefore often test for exact error messages. However, this only applies to errors generated by Node.js core. For errors generated by V8, the TSC has determined that we do not consider it a breaking change when an error message changes.

We have a relatively easy way to find these errors in our tests because ChakraCore will often (usually? almost always?) need to modify such tests to match their error messages.

This PR changes several tests that node-chakracore needed to modify due to the overzealous error message checking (much of which I may have been responsible for either directly or indirectly, but hey, we all get to learn from our mistakes).

There are still more tests to be done, but this is a good start and reaction to this will inform the approach in the rest of the tests.

@nodejs/chakracore @nodejs/testing

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

test

Trott · 2017-10-18T00:07:14Z

CI: https://ci.nodejs.org/job/node-test-pull-request/10796/

digitalinfinity

This is awesome- thanks for this!

boingoing

This is great!

💯 👍

targos · 2017-10-18T01:15:25Z

test/parallel/test-cli-syntax.js

@@ -13,7 +13,9 @@ const syntaxArgs = [
  ['--check']
 ];

-const syntaxErrorRE = /^SyntaxError: Unexpected identifier$/m;
+// Match on the name of the `Error` but not the message as it is different
+// depending on the aScript engine (V8, ChakraCore, etc.)


typo: JavaScript engine

@targos Fixed!

refack · 2017-10-18T01:28:12Z

test/parallel/test-internal-util-decorate-error-stack.js

+  // displays the line of code (`var foo bar;`) that is causing a problem.
+  // ChakraCore does not display the line of code but includes `;` in the phrase
+  // `Expected ';' `.
+  assert.ok(/;/g.test(stack));


Is this the best we can do?
Can we at least test that it's a multiline string?

@refack I guess that's something. I've added this:

// Test that it's a multiline string. assert.ok(/\n/g.test(stack));

hiroppy

LGTM🍻

joyeecheung

LGTM, although I wouldn't even mention "V8 and ChakraCore", just say "different JavaScript engines"...

lpinca · 2017-10-18T10:41:03Z

test/parallel/test-error-reporting.js

@@ -32,17 +32,14 @@ function errExec(script, callback) {
    assert.ok(err);

    // More than one line of error output.


Nit: should be "At least one line ... " or the assertion should be > 1.

cjihrig

LGTM. I agree with @joyeecheung that mentioning the engines by name is not necessary.

jasnell · 2017-10-18T15:27:01Z

test/parallel/test-console-count.js

 assert.throws(
  () => console.count(Symbol('test')),
-  /^TypeError: Cannot convert a Symbol value to a string$/);


@Trott, I know it's a ton of tedious work, but can these assert.throws() be modified to common.expectsError() for sake of consistency?

e.g.

common.expectsError( () => console.count(Symbol('test')), { type: TypeError } );

(feel free to say no for doing so in this PR :-) ...)

Sounds like a great code-and-learn PR

trivikr

LGTM

gireeshpunathil · 2017-10-19T09:58:48Z

@Trott
Forgive me for raising voice on a PR that is the result of a great work, but thought I should mention this here. In an effort to resolve the vm-neutrality gaps, we seem to address it at it's manifestation stage, not at the root - with a side effect of relaxing error checks to their most abstract form. TypeError by far is the most common error in Javascript, and stripping the exact details at error sites may cause some of the real bugs to escape the tests?

Given that

TSC has determined that we do not consider it a breaking change when an error message changes.

Should we work with v8 and chakra to unify their error messages?

joyeecheung · 2017-10-19T17:27:15Z

@gireeshpunathil Unified messages seem to fall into the scope of ECMAScript spec, and the spec usually only specifies the type of the error and leave the message to the vendors, which is basically what this PR tries to do as well.

Do not check the error message if it is generated by the JavaScript engine (V8, ChakraCore, etc.). Do confirm that it is a `RangeError`.

Do not check the error message if it is generated by the JavaScript engine (V8, ChakraCore, etc.). Do confirm that it is a `SyntaxError`.

Do not check the error message if it is generated by the JavaScript engine (V8, ChakraCore, etc.). Do confirm that it is a `TypeError`.

Remove or modify checks in test-error-reporting that only apply to V8 and not to ChakraCore.

Do not check the error message if it is generated by the JavaScript engine (V8, ChakraCore, etc.). Do confirm that it is a `TypeError`.

Revise test-internal-util-decorate-error-stack so that it does not depend on behavior specific to V8.

Do not check the error message if it is generated by the JavaScript engine (V8, ChakraCore, etc.). Do confirm that it is a `TypeError`.

Do not check the error message if it is generated by the JavaScript engine (V8, ChakraCore, etc.). Do confirm that it is a `SyntaxError`. PR-URL: #16272 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>

Remove reliance on V8-specific error messages in test/parallel/test-tls-external-accessor.js. Check that the error is a `TypeError`. The test should now be successful without modification using ChakraCore. PR-URL: nodejs#16272 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>

Remove reliance on V8-specific error messages in test/parallel/test-tls-external-accessor.js. Check that the error is a `TypeError`. The test should now be successful without modification using ChakraCore. Backport-PR-URL: #20456 PR-URL: #16272 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>

Remove reliance on V8-specific error messages in test/parallel/test-tls-external-accessor.js. Check that the error is a `TypeError`. The test should now be successful without modification using ChakraCore. Backport-PR-URL: nodejs/node#20456 PR-URL: nodejs/node#16272 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>

* src: replace SetAccessor w/ SetAccessorProperty PR-URL: nodejs/node#17665 Fixes: nodejs/node#17636 Refs: nodejs/node#16482 Refs: nodejs/node#16860 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> * test: make test-tls-external-accessor agnostic Remove reliance on V8-specific error messages in test/parallel/test-tls-external-accessor.js. Check that the error is a `TypeError`. The test should now be successful without modification using ChakraCore. Backport-PR-URL: nodejs/node#20456 PR-URL: nodejs/node#16272 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>

Remove or modify checks in test-error-reporting that only apply to V8 and not to ChakraCore. PR-URL: #16272 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>

Trott added the test Issues and PRs related to the tests. label Oct 18, 2017

digitalinfinity approved these changes Oct 18, 2017

View reviewed changes

boingoing approved these changes Oct 18, 2017

View reviewed changes

targos approved these changes Oct 18, 2017

View reviewed changes

refack approved these changes Oct 18, 2017

View reviewed changes

hiroppy approved these changes Oct 18, 2017

View reviewed changes

joyeecheung approved these changes Oct 18, 2017

View reviewed changes

lpinca approved these changes Oct 18, 2017

View reviewed changes

cjihrig approved these changes Oct 18, 2017

View reviewed changes

jasnell reviewed Oct 18, 2017

View reviewed changes

jasnell approved these changes Oct 18, 2017

View reviewed changes

trivikr approved these changes Oct 19, 2017

View reviewed changes

benjamingr approved these changes Oct 19, 2017

View reviewed changes

tniessen approved these changes Oct 19, 2017

View reviewed changes

Trott added 2 commits November 11, 2017 14:44

test: make test-buffer-slow engine agnostic

9231450

Do not check the error message if it is generated by the JavaScript engine (V8, ChakraCore, etc.). Do confirm that it is a `RangeError`.

test: make test-cli-syntax engine agnostic

12cc6c0

Do not check the error message if it is generated by the JavaScript engine (V8, ChakraCore, etc.). Do confirm that it is a `SyntaxError`.

Trott force-pushed the engine-agnostic branch 2 times, most recently from 8e0c674 to 157c029 Compare November 11, 2017 22:53

Trott added 9 commits November 11, 2017 15:00

test: make test-console-count engine agnostic

8bde5ed

Do not check the error message if it is generated by the JavaScript engine (V8, ChakraCore, etc.). Do confirm that it is a `TypeError`.

test: make test-console engine agnostic

2ab4577

Do not check the error message if it is generated by the JavaScript engine (V8, ChakraCore, etc.). Do confirm that it is a `TypeError`.

test: make test-error-reporting engine agnostic

e8e6863

Remove or modify checks in test-error-reporting that only apply to V8 and not to ChakraCore.

test: make test-http-outgoing-proto agnostic

53c033c

Do not check the error message if it is generated by the JavaScript engine (V8, ChakraCore, etc.). Do confirm that it is a `TypeError`.

test: make error stack test engine agnostic

ab32e20

Revise test-internal-util-decorate-error-stack so that it does not depend on behavior specific to V8.

test: make test-os-eol engine agnostic

b64b12e

Do not check the error message if it is generated by the JavaScript engine (V8, ChakraCore, etc.). Do confirm that it is a `TypeError`.

test: make test-process-env-symbols agnostic

ebc145f

Do not check the error message if it is generated by the JavaScript engine (V8, ChakraCore, etc.). Do confirm that it is a `TypeError`.

test: make test-querystring-escape engine agnostic

c40983b

Do not check the error message if it is generated by the JavaScript engine (V8, ChakraCore, etc.). Do confirm that it is a `TypeError`.

test: make test-repl-harmony engine agnostic

ef2da79

Do not check the error message if it is generated by the JavaScript engine (V8, ChakraCore, etc.). Do confirm that it is a `TypeError`.

Trott removed the backport-requested-v8.x label Nov 17, 2017

MylesBorins mentioned this pull request Feb 13, 2018

v6.13.0 proposal #18342

Merged

MylesBorins mentioned this pull request May 2, 2018

v8.11.2 proposal #20478

Merged

jasonrudolph mentioned this pull request Jun 15, 2018

src: replace SetAccessor w/ SetAccessorProperty electron/node#38

Merged

MylesBorins mentioned this pull request Jul 9, 2018

v8.12.0 proposal #21593

Merged

Trott deleted the engine-agnostic branch January 13, 2022 22:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make tests engine agnostic #16272

Make tests engine agnostic #16272

Trott commented Oct 18, 2017

Trott commented Oct 18, 2017

digitalinfinity left a comment

boingoing left a comment

targos Oct 18, 2017

Trott Nov 11, 2017

refack Oct 18, 2017

Trott Nov 11, 2017

refack Nov 11, 2017

hiroppy left a comment

joyeecheung left a comment

lpinca Oct 18, 2017

cjihrig left a comment

jasnell Oct 18, 2017

jasnell Oct 18, 2017

gibfahn Oct 18, 2017

trivikr left a comment

gireeshpunathil commented Oct 19, 2017

joyeecheung commented Oct 19, 2017

		@@ -32,17 +32,14 @@ function errExec(script, callback) {
		assert.ok(err);

		// More than one line of error output.

Make tests engine agnostic #16272

Make tests engine agnostic #16272

Conversation

Trott commented Oct 18, 2017

Checklist

Affected core subsystem(s)

Trott commented Oct 18, 2017

digitalinfinity left a comment

Choose a reason for hiding this comment

boingoing 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

hiroppy left a comment

Choose a reason for hiding this comment

joyeecheung left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjihrig 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

trivikr left a comment

Choose a reason for hiding this comment

gireeshpunathil commented Oct 19, 2017

joyeecheung commented Oct 19, 2017