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

test: test-assert.js fails locally #18967

Closed
vsemozhetbyt opened this issue Feb 23, 2018 · 6 comments
Closed

test: test-assert.js fails locally #18967

vsemozhetbyt opened this issue Feb 23, 2018 · 6 comments
Labels
assert Issues and PRs related to the assert subsystem. test Issues and PRs related to the tests. windows Issues and PRs related to the Windows platform.

Comments

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Feb 23, 2018

  • Version: 10.0.0 (master)
  • Platform: Windows 7 x64
  • Subsystem: test. assert

Testing the #18904 locally, I've got this not related error, which I can also reproduce with the last nightly:

assert.js:74
  throw new AssertionError(obj);
  ^

AssertionError [ERR_ASSERTION]: 'The expression evaluated to a falsy value:\r\n\r\n  assert((() => \'string\')()\n      // eslint-disable-next-line\n      ==... strictEqual 'The expression evaluated to a falsy value:\r\n\r\n  assert((() => \'string\')()\r\n      // eslint-disable-next-line\r\n    ...
    at Object.innerFn (test\common\index.js:701:16)
    at expectedException (assert.js:406:19)
    at Function.throws (assert.js:451:16)
    at Object.expectsError (test\common\index.js:723:12)
    at Object.<anonymous> (test\parallel\test-assert.js:642:8)
    at Module._compile (module.js:666:30)
    at Object.Module._extensions..js (module.js:677:10)
    at Module.load (module.js:577:32)
    at tryModuleLoad (module.js:517:12)
    at Function.Module._load (module.js:509:3)

It seems reasonable: this fragment uses \n on all systems, while this fragment uses \r\n on Windows.

Why does not this test fail on CI?

@vsemozhetbyt vsemozhetbyt added assert Issues and PRs related to the assert subsystem. windows Issues and PRs related to the Windows platform. test Issues and PRs related to the tests. labels Feb 23, 2018
@BridgeAR
Copy link
Member

That is a very interesting case. It seems like the parsed output from acorn is always using \n. The test uses the OS EOL and tests against this.
Is there any chance that the acorn version is not the same for you as the one in the CI? Tomorrow I will check what acorn will use a default.

@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt commented Feb 24, 2018

Is there any chance that the acorn version is not the same for you as the one in the CI?

I cannot think of any cause for this)

@nodejs/platform-windows, can anybody else check the test locally or suppose any causes for this difference?

@vsemozhetbyt
Copy link
Contributor Author

Still fails.

@bzoz
Copy link
Contributor

bzoz commented May 8, 2018

Pipeing test output fixes this, node --expose-internals test\parallel\test-assert.js | more passes.

@targos
Copy link
Member

targos commented May 8, 2018

I also experienced this on Windows locally.

@bzoz
Copy link
Contributor

bzoz commented May 8, 2018

I guess the issue is here:

node/lib/internal/errors.js

Lines 403 to 407 in dd03709

if (process.stdout.getColorDepth() !== 1) {
blue = '\u001b[34m';
green = '\u001b[32m';
white = '\u001b[39m';
red = '\u001b[31m';

When the stdout is a TTY, colors are added to the error message and this confuses test runner. We could add a way to disable colors, or make the test expect them. Or, we can make the test spawn itself with piped output, so the error messages will get generated without colors.

cjihrig added a commit to cjihrig/node that referenced this issue May 15, 2018
When test/parallel/test-assert.js is run with a TTY as stdout,
color codes in assertion messages cause the test to fail. This
commit disables colors when stdout is a TTY.

Fixes: nodejs#18967
PR-URL: nodejs#20695
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
bzoz added a commit to JaneaSystems/node that referenced this issue May 15, 2018
Adds a note to test/README.md that setting autocrlf to true when
checking out sources is required for the tests to run successfully.

Ref: nodejs#18967
apapirovski pushed a commit that referenced this issue May 22, 2018
Adds a note to test/README.md that setting autocrlf to true when
checking out sources is required for the tests to run successfully.

PR-URL: #20752
Ref: #18967
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
MylesBorins pushed a commit that referenced this issue May 22, 2018
When test/parallel/test-assert.js is run with a TTY as stdout,
color codes in assertion messages cause the test to fail. This
commit disables colors when stdout is a TTY.

Fixes: #18967
PR-URL: #20695
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue May 22, 2018
Adds a note to test/README.md that setting autocrlf to true when
checking out sources is required for the tests to run successfully.

PR-URL: #20752
Ref: #18967
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
MylesBorins pushed a commit that referenced this issue May 23, 2018
Adds a note to test/README.md that setting autocrlf to true when
checking out sources is required for the tests to run successfully.

PR-URL: #20752
Ref: #18967
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assert Issues and PRs related to the assert subsystem. test Issues and PRs related to the tests. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

No branches or pull requests

4 participants