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

WriteStream.prototype.hasColors() support #971

Closed
mrazauskas opened this issue Dec 21, 2022 · 6 comments
Closed

WriteStream.prototype.hasColors() support #971

mrazauskas opened this issue Dec 21, 2022 · 6 comments
Labels
need-discussion needs more in-depth discussion

Comments

@mrazauskas
Copy link
Contributor

mrazauskas commented Dec 21, 2022

Environment

  1. vscode-jest version: 5.1.0
  2. node -v: 19.3.0
  3. npm -v or yarn --version: 3.3.1
  4. npm ls jest or npm ls react-scripts (if you haven’t ejected): [email protected]
  5. your vscode-jest settings if customized:
    • jest.jestCommandLine? yarn test (see reproduction repo)
  6. Operating system: MacOS

Prerequisite

  • are you able to run jest test from the command line? Yes
  • how do you run your tests from the command line? yarn test

Steps to Reproduce

Reproduction repo is here: https://github.com/mrazauskas/x-has-colors

Expected Behavior

Same test result in Jest CLI and vscode-jest.

Actual Behavior

The snapshot is passing via CLI, but failing via vscode-jest.

The reproduction is using Node’s build-in WriteStream.prototype.hasColors() method (see documentation). Seems like vscode-jest has some issue with support of this API. I decided to open a ticket, because that is Node’s native API.

EDIT: See comment below.

You may notice that Jest is patched in the reproduction repo. The patch makes Jest to use Worker Threads. It has no impact in this case, because there is just one test suite (workers will not ne used).

I left the patch just to draw your attention that Jest has similar issue with Node’s hasColors(). When workers are used (Child Process), the env is overwritten here: https://github.com/facebook/jest/blob/fd8f89719912996802fe90bdeab1d5a6d8e95d8e/packages/jest-worker/src/workers/ChildProcessWorker.ts#L132-L136

As you will see in the code supports-color is used there, but supports-color does not respect Node’s environmental variables NODE_DISABLE_COLORS and NO_COLORS. In contrary hasColors() takes those into account. This is why support of hasColors() is broken in Jest’s Child Process workers. I can’t find a smart way to fix that, swapping to Worker Threads with a patch was better idea.

I was tying to figure out if maybe vscode-jest is doing something unexpected with environment variables as well. Got swamped in the codebase, found nothing relevant. Hopeful all what I wrote here will make it easier to spot the issue for someone familiar with the code (;

@mrazauskas
Copy link
Contributor Author

mrazauskas commented Dec 22, 2022

One more idea. I just checked process.env and found out that COLORTERM: 'truecolor' is present then Jest runs via CLI, but it is gone if vscode-jest runs the tests. Strange.

As a work around I have set "jest.jestCommandLine": "FORCE_COLOR='' yarn test".

@mrazauskas
Copy link
Contributor Author

mrazauskas commented Dec 22, 2022

Would it be possible somehow to use FORCE_COLOR=1 instead of --colors flag here:

reporters: ['default', `"${this.getReporterPath()}"`],
args: { args: ['--colors'] },
};

Found it by activating debugMode and looking at the output in Developer Tools. process.env in the spawned process belongs to Electron. That’s why variables are different from the ones in Terminal.

FORCE_COLOR=1 environmental variable and --colors flag are equal for Jest. The variable or flag are only consumed by Chalk and both are supported: https://github.com/chalk/chalk#supportscolor

Node and WriteStream.prototype.hasColors() respect only FORCE_COLOR. Having it set should solve the issue.

@connectdotz
Copy link
Collaborator

@mrazauskas, thanks for your research; it helped a lot!

Looks like we could add the COLORTERM: 'truecolor' to the process.env, as the vscode terminal does here.

I am glad you found a workaround, another one is to use the jest.nodeEnv setting:

"jest.nodeEnv": { "COLORTERM": "truecolor" }

@mrazauskas
Copy link
Contributor Author

Right, it would be better to add COLORTERM: 'truecolor' to process.env.

Would it be good idea to add env to options object of Runner class from jest-editor-support? It should pass it to createProcess, of course. This way a consumer of the API like vscode-jest could set any variables it needs.

If that sounds good, I could do this after holidays.

@connectdotz
Copy link
Collaborator

Would it be good idea to add env to options object of Runner class from jest-editor-support?

It already is; that is how we pass along the setting jest.nodeEnv (here).

The question is if the extension should add it, or leave it to the users as customization when needed. This use case seems to be relatively rare as you are the first user ever bumped into this issue, and I am not sure if there is any ill side-effect if we add it to all use cases...

Let's do this: Giving there is an existing setting that you can use, I will not change anything for now. But we will leave this issue open, and if more similar user cases arise, we can certainly reconsider.

@connectdotz connectdotz added need-discussion needs more in-depth discussion and removed enhancement labels Dec 25, 2022
@mrazauskas
Copy link
Contributor Author

What about mocking hasColors in my tests:

jest.spyOn(WriteStream.prototype, "hasColors").mockReturnValue(true);

Simple and makes them run in any environment. I think that is better solution than keeping tests dependent on some environmental variables. At first it wasn’t clear why tests are falling, but now I think my tests were simply too fragile and needed a refactor.

Thanks for your comments!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need-discussion needs more in-depth discussion
Projects
None yet
Development

No branches or pull requests

2 participants