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

test: general improvements to vm tests #14458

Closed
wants to merge 10 commits into
base: master
from

Conversation

Projects
None yet
@jasnell
Member

jasnell commented Jul 24, 2017

General improvements to vm tests

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test

Show outdated Hide outdated test/parallel/test-vm-context.js

@mscdex mscdex added the vm label Jul 24, 2017

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Jul 24, 2017

Member

Not objecting enough to stop it, but I'm not a fan of removing console.* calls in tests as a supposed improvement. Those are often super helpful when tests start failing in CI in ways that are difficult to reproduce locally. Seems to favor abstract ideas about how tests are "supposed" to be written over practical considerations.

Member

Trott commented Jul 24, 2017

Not objecting enough to stop it, but I'm not a fan of removing console.* calls in tests as a supposed improvement. Those are often super helpful when tests start failing in CI in ways that are difficult to reproduce locally. Seems to favor abstract ideas about how tests are "supposed" to be written over practical considerations.

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Jul 24, 2017

Member

I've been considering introducing a new common.log() that would be a non-op unless a NODE_TEST_LOG=1 environment variable is set... that would accomplish the same basic thing without introducing the unnecessary noise by default.

Member

jasnell commented Jul 24, 2017

I've been considering introducing a new common.log() that would be a non-op unless a NODE_TEST_LOG=1 environment variable is set... that would accomplish the same basic thing without introducing the unnecessary noise by default.

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Jul 24, 2017

Member

I've been considering introducing a new common.log() that would be a non-op unless a NODE_TEST_LOG=1 environment variable is set... that would accomplish the same basic thing without introducing the unnecessary noise by default.

What is the downside of console.log() statements that we are trying to avoid? What is the value we get in return for the added complexity?

(Feel free to take this offline if it's a distraction from the PR generally.)

Member

Trott commented Jul 24, 2017

I've been considering introducing a new common.log() that would be a non-op unless a NODE_TEST_LOG=1 environment variable is set... that would accomplish the same basic thing without introducing the unnecessary noise by default.

What is the downside of console.log() statements that we are trying to avoid? What is the value we get in return for the added complexity?

(Feel free to take this offline if it's a distraction from the PR generally.)

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Jul 25, 2017

Member

most of the time the console.log output is generally quite useless in the tests that exist currently. For instance, printing console.log('ok') unconditionally when the test exits, or calling console.log() within unnecessary event handlers, with content that is not very useful, even for simple debugging.

Member

jasnell commented Jul 25, 2017

most of the time the console.log output is generally quite useless in the tests that exist currently. For instance, printing console.log('ok') unconditionally when the test exits, or calling console.log() within unnecessary event handlers, with content that is not very useful, even for simple debugging.

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Jul 25, 2017

Member

most of the time the console.log output is generally quite useless in the tests that exist currently. For instance, printing console.log('ok') unconditionally when the test exits, or calling console.log() within unnecessary event handlers, with content that is not very useful, even for simple debugging.

If most of the logging is useless (which is an assessment I do not agree with), how does creating a whole new way to control logging improve anything? It seems that it would be better to improve or remove existing log messages.

Member

Trott commented Jul 25, 2017

most of the time the console.log output is generally quite useless in the tests that exist currently. For instance, printing console.log('ok') unconditionally when the test exits, or calling console.log() within unnecessary event handlers, with content that is not very useful, even for simple debugging.

If most of the logging is useless (which is an assessment I do not agree with), how does creating a whole new way to control logging improve anything? It seems that it would be better to improve or remove existing log messages.

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Jul 25, 2017

Member

Although looking more at these specific logging messages...yeah, not sure I get the utility of them. Sorry I said anything here. I should have saved it for something else where the log messages might have value. 😆

Member

Trott commented Jul 25, 2017

Although looking more at these specific logging messages...yeah, not sure I get the utility of them. Sorry I said anything here. I should have saved it for something else where the log messages might have value. 😆

@Trott

Trott approved these changes Jul 25, 2017

LGTM if CI is green.

@targos

targos approved these changes Jul 25, 2017

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Jul 25, 2017

Member

@Trott ... I definitely understand where you're coming from. Introducing a new common.log() type of mechanism and converting over gives us an opportunity to do two things: 1) improve the existing output messages to make them more useful and 2) improve the tests overall. As is evidenced by the recent round of test improvements I've been making, there are a range of improvements that can be made.

Member

jasnell commented Jul 25, 2017

@Trott ... I definitely understand where you're coming from. Introducing a new common.log() type of mechanism and converting over gives us an opportunity to do two things: 1) improve the existing output messages to make them more useful and 2) improve the tests overall. As is evidenced by the recent round of test improvements I've been making, there are a range of improvements that can be made.

@Trott

Needs nits from @lpinca before landing.

@Trott Trott referenced this pull request Jul 29, 2017

Closed

test: refactor test-vm-new-script-new-context #14536

2 of 2 tasks complete
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Jul 31, 2017

Member

Updated, rebased

Member

jasnell commented Jul 31, 2017

Updated, rebased

@Trott Trott dismissed their stale review Aug 1, 2017

updated since review, dismissing request for changes

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell
Member

jasnell commented Aug 2, 2017

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Aug 2, 2017

Member

Canceled the CI run... there appear to be some issues in CI-land (ping @nodejs/build)

Member

jasnell commented Aug 2, 2017

Canceled the CI run... there appear to be some issues in CI-land (ping @nodejs/build)

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell
Member

jasnell commented Aug 2, 2017

@fhinkel

fhinkel approved these changes Aug 3, 2017

jasnell added a commit that referenced this pull request Aug 3, 2017

test: improve multiple vm tests
PR-URL: #14458
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Aug 3, 2017

Member

Landed 4b23b42

Member

jasnell commented Aug 3, 2017

Landed 4b23b42

@jasnell jasnell closed this Aug 3, 2017

MylesBorins added a commit that referenced this pull request Sep 9, 2017

test: improve multiple vm tests
PR-URL: #14458
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

@MylesBorins MylesBorins referenced this pull request Sep 10, 2017

Merged

v8.5.0 proposal #15308

MylesBorins added a commit that referenced this pull request Sep 20, 2017

test: improve multiple vm tests
PR-URL: #14458
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

@MylesBorins MylesBorins referenced this pull request Sep 20, 2017

Merged

v6.11.4 proposal #15506

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment