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.

Sign up for GitHub

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

Jump to bottom

test: assert diff no color #24181

Closed
wants to merge 2 commits into from
Closed

test: assert diff no color #24181

wants to merge 2 commits into from

Conversation

fbilbie
Copy link
Contributor

@fbilbie fbilbie commented Nov 6, 2018 •
edited
Loading

Checklist

This pull request adds a test that covers lines 313-316 from root/internal assert.js. Specifically it tests the scenario when the tty does not support colors and the assert messages should not be decorated.

I emulated the scenario by setting the environment variable NODE_DISABLE_COLORS to true

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

@nodejs-github-bot nodejs-github-bot added test Issues and PRs related to the tests. tty Issues and PRs related to the tty subsystem. labels Nov 6, 2018
@addaleax addaleax added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Nov 6, 2018
@addaleax
Copy link
Member

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

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 10, 2018
Trott
Trott reviewed Nov 11, 2018
View reviewed changes
Copy link
Member

@Trott Trott left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Can you add a comment explaining what is being tested? The test passes (in my environment at least) even without the NODE_DISABLE_COLORS or with that environment variable set to false. (EDIT: More explanation below about why the test is passing regardless of env variable setting.)

Trott
Trott reviewed Nov 11, 2018
View reviewed changes
test/pseudo-tty/test-assert-no-color.js Outdated
process.env.NODE_DISABLE_COLORS = true;

try {
assert.strictEqual(1, 3);
Copy link
Member

Choose a reason for hiding this comment

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

This won't normally result in colorized output, at least not in my environment. assert.deepStrictEqual({a: 'a'}, {}) will do the trick.

Trott
Trott previously requested changes Nov 11, 2018
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

This test is not quite correct, I don't think. See comments.

@Trott
Copy link
Member

Trott commented Nov 11, 2018

OK, so the assert.strictEqual(1,3) won't normally result in colorized output, so the test will pass whether or not the environment variable is set.

To get colorized output, maybe try something like assert.deepStrictEqual([1],[3]) instead. Then, the environment variable will have an effect and changing it will cause the test to fail as expected.

@Trott Trott removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 11, 2018
@fbilbie
Copy link
Contributor Author

fbilbie commented Nov 12, 2018 •
edited
Loading

Hi @Trott. Thanks for pointing that out. I did a rocky mistake. When I first ran the test it was with a node version installed locally (10.13.0) not with the version from master. That version had colors and a different text on strictEqual. When I found out that I had to run with ./node i did not checked to see if the output has colors; I changed only the text.

I will update the pull request and add some description about the test case

@fbilbie
Copy link
Contributor Author

fbilbie commented Nov 13, 2018

I made the changes.

@Trott Trott dismissed their stale review November 13, 2018 16:05

changes made

@Trott
Copy link
Member

Trott commented Nov 13, 2018

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

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 13, 2018
@Trott
Copy link
Member

Trott commented Nov 13, 2018

Resume Build CI: https://ci.nodejs.org/job/node-test-pull-request/18585/

@Trott
Copy link
Member

Trott commented Nov 13, 2018

Landed in a2c2c78

@Trott Trott closed this Nov 13, 2018
Trott pushed a commit to Trott/io.js that referenced this pull request Nov 13, 2018
@fbilbie @Trott
test: assert diff no color
a2c2c78 
PR-URL: nodejs#24181
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR pushed a commit that referenced this pull request Nov 14, 2018
@fbilbie @BridgeAR
test: assert diff no color
aa800b0 
PR-URL: #24181
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Nov 14, 2018
Merged
kiyomizumia pushed a commit to kiyomizumia/node that referenced this pull request Nov 15, 2018
@fbilbie @kiyomizumia
test: assert diff no color
470d55a 
PR-URL: nodejs#24181
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
This was referenced Nov 15, 2018
Closed
Closed
Closed
This was referenced Nov 16, 2018
Closed
Closed
Closed
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

@addaleax addaleax addaleax approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@Trott Trott Trott left review comments

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. test Issues and PRs related to the tests. tty Issues and PRs related to the tty subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@fbilbie @addaleax @Trott @jasnell @BridgeAR @codebytere @nodejs-github-bot