GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Make tests engine agnostic by Trott · Pull Request #16272 · nodejs/node

Conversation

@Trott

@Trott Trott commented Oct 18, 2017

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
maclover7, tuananh, and benjamingr reacted with thumbs up emoji boingoing reacted with hooray emoji digitalinfinity reacted with heart emoji
All reactions
  • 👍 3 reactions
  • 🎉 1 reaction
  • ❤️ 1 reaction
@Trott Trott added the test Issues and PRs related to Node.js core tests and test infrastructure. label Oct 18, 2017
@Trott

Trott commented Oct 18, 2017

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
digitalinfinity
digitalinfinity approved these changes Oct 18, 2017
View reviewed changes

@digitalinfinity digitalinfinity left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This is awesome- thanks for this!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
boingoing
boingoing approved these changes Oct 18, 2017
View reviewed changes

@boingoing boingoing left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This is great!

💯 👍

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
targos
targos approved these changes Oct 18, 2017
View reviewed changes
Comment thread test/parallel/test-cli-syntax.js Outdated

@targos targos Oct 18, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

typo: JavaScript engine

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@Trott Trott Nov 11, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

@targos Fixed!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
refack
refack approved these changes Oct 18, 2017
View reviewed changes
Comment thread test/parallel/test-internal-util-decorate-error-stack.js Outdated

@refack refack Oct 18, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@Trott Trott Nov 11, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
refack reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@refack refack Nov 11, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

💯

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
hiroppy
hiroppy approved these changes Oct 18, 2017
View reviewed changes

@hiroppy hiroppy left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM🍻

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
joyeecheung
joyeecheung approved these changes Oct 18, 2017
View reviewed changes

@joyeecheung joyeecheung left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
lpinca
lpinca approved these changes Oct 18, 2017
View reviewed changes
Comment thread test/parallel/test-error-reporting.js Outdated

@lpinca lpinca Oct 18, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
cjihrig
cjihrig approved these changes Oct 18, 2017
View reviewed changes

@cjihrig cjihrig left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
jasnell
jasnell reviewed Oct 18, 2017
View reviewed changes
Comment thread test/parallel/test-console-count.js Outdated

@jasnell jasnell Oct 18, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

@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 }
);

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@jasnell jasnell Oct 18, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@gibfahn gibfahn Oct 18, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Sounds like a great code-and-learn PR

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
jasnell, lpinca, joyeecheung, trivikr, and maclover7 reacted with thumbs up emoji jasnell and benjamingr reacted with laugh emoji
All reactions
  • 👍 5 reactions
  • 😄 2 reactions
jasnell
jasnell approved these changes Oct 18, 2017
View reviewed changes
trivikr
trivikr approved these changes Oct 19, 2017
View reviewed changes

@trivikr trivikr left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@gireeshpunathil

gireeshpunathil commented Oct 19, 2017

Copy link
Copy Markdown
Member

@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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
benjamingr
benjamingr approved these changes Oct 19, 2017
View reviewed changes
tniessen
tniessen approved these changes Oct 19, 2017
View reviewed changes
@joyeecheung

joyeecheung commented Oct 19, 2017

Copy link
Copy Markdown
Member

@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.
Trott, gireeshpunathil, and digitalinfinity reacted with thumbs up emoji
All reactions
  • 👍 3 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Trott added 2 commits November 11, 2017 14:44
@Trott
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`.
@Trott
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
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
@Trott
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`.
@Trott
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`.
@Trott
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.
@Trott
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`.
@Trott
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.
@Trott
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`.
@Trott
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`.
@Trott
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`.
@Trott
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`.
evanlucas pushed a commit that referenced this pull request Nov 13, 2017
@Trott @evanlucas
test: make test-repl-syntax-error-stack agnostic
f8337ce 
Revise test so that it does not depend on behavior specific to V8.

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]>
evanlucas pushed a commit that referenced this pull request Nov 13, 2017
@Trott @evanlucas
test: make test-repl engine agnostic
835ca63 
Remove reliance on V8-specific error messages in test-repl. Test should
pass whether V8 is the engine, or V8 changes error message strings, or
ChakraCore is the engine, or some other valid JavaScript engine is used
in the future.

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]>
evanlucas pushed a commit that referenced this pull request Nov 13, 2017
@Trott @evanlucas
test: make test-require-json engine agnostic
0be7f8c 
Remove reliance on V8-specific error message in test-require-json.

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]>
evanlucas pushed a commit that referenced this pull request Nov 13, 2017
@Trott @evanlucas
test: make test-tls-external-accessor agnostic
3136578 
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: #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]>
@evanlucas evanlucas mentioned this pull request Nov 13, 2017
v9.2.0 proposal #16992
Merged
@MylesBorins

MylesBorins commented Nov 16, 2017

Copy link
Copy Markdown
Contributor

Should this be backported to v6.x-staging or v8.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@MylesBorins MylesBorins added backport-requested-v6.x labels Nov 16, 2017
@Trott Trott added dont-land-on-v4.x and removed backport-requested-v6.x labels Nov 17, 2017
@MylesBorins MylesBorins mentioned this pull request Feb 13, 2018
v6.13.0 proposal #18342
Merged
@MylesBorins MylesBorins mentioned this pull request May 2, 2018
v8.11.2 proposal #20478
Merged
@jasonrudolph jasonrudolph mentioned this pull request Jun 15, 2018
src: replace SetAccessor w/ SetAccessorProperty electron/node#38
Merged
@MylesBorins MylesBorins mentioned this pull request Jul 9, 2018
v8.12.0 proposal #21593
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes
@benjamingr benjamingr benjamingr approved these changes
@lpinca lpinca lpinca approved these changes
@cjihrig cjihrig cjihrig approved these changes
@tniessen tniessen tniessen approved these changes
@joyeecheung joyeecheung joyeecheung approved these changes
@trivikr trivikr trivikr approved these changes
+6 more reviewers
@refack refack refack approved these changes
@gibfahn gibfahn gibfahn left review comments
@digitalinfinity digitalinfinity digitalinfinity approved these changes
@boingoing boingoing boingoing approved these changes
@targos targos targos approved these changes
@hiroppy hiroppy hiroppy approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

test Issues and PRs related to Node.js core tests and test infrastructure.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

16 participants

@Trott @gireeshpunathil @joyeecheung @jasnell @MylesBorins @refack @digitalinfinity @benjamingr @lpinca @boingoing @targos @cjihrig @tniessen @hiroppy @gibfahn @trivikr
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.