镜像站点 · 本页由第三方 GitHub 只读镜像提供,非 GitHub 官方站点,不接受任何登录或凭据输入。前往 github.com
Skip to content

util: allow colors in non-tty if FORCE_COLOR - #54289

Closed
avivkeller wants to merge 1 commit into
nodejs:mainfrom
avivkeller:tty-force-colors
Closed

avivkeller wants to merge 1 commit into
nodejs:mainfrom
avivkeller:tty-force-colors

Conversation

@avivkeller

@avivkeller avivkeller commented Aug 9, 2024 •

Copy link
Copy Markdown
Member

Fixes #52249

internal/util/colors has been ignoring FORCE_COLOR if the terminal is not a TTY terminal. This PR changes that behavior to allow FORCE_COLOR to, well, force colors.;

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module. labels Aug 9, 2024
@codecov

codecov Bot commented Aug 9, 2024 •

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.33%. Comparing base (3cbeed8) to head (d1c912f).
Report is 485 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #54289      +/-   ##
==========================================
+ Coverage   87.11%   87.33%   +0.21%     
==========================================
  Files         647      649       +2     
  Lines      181773   182603     +830     
  Branches    34889    35041     +152     
==========================================
+ Hits       158347   159469    +1122     
+ Misses      16739    16400     -339     
- Partials     6687     6734      +47     
Files with missing lines Coverage Δ
lib/internal/util/colors.js 100.00% <100.00%> (ø)

... and 107 files with indirect coverage changes

@avivkeller avivkeller added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Aug 10, 2024
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Aug 10, 2024
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@avivkeller

Copy link
Copy Markdown
Member Author

@avivkeller

Copy link
Copy Markdown
Member Author

CI problems should be fixed. There were cases where the hasColors value was false for other reasons, now it checks that it matches shouldColorize.

@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Aug 16, 2024
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Aug 16, 2024
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@avivkeller

Copy link
Copy Markdown
Member Author

I've noticed this error on multiple CI's:

ERROR: Error fetching remote repo 'origin'
hudson.plugins.git.GitException: Failed to fetch from git@github.com:nodejs/node.git
	at PluginClassLoader for git//hudson.plugins.git.GitSCM.fetchFrom(GitSCM.java:997)
	at PluginClassLoader for git//hudson.plugins.git.GitSCM.retrieveChanges(GitSCM.java:1239)
	at PluginClassLoader for git//hudson.plugins.git.GitSCM.checkout(GitSCM.java:1303)
	at hudson.scm.SCM.checkout(SCM.java:540)
	at hudson.model.AbstractProject.checkout(AbstractProject.java:1247)
	at hudson.model.AbstractBuild$AbstractBuildExecution.defaultCheckout(AbstractBuild.java:649)
	at jenkins.scm.SCMCheckoutStrategy.checkout(SCMCheckoutStrategy.java:85)
	at hudson.model.AbstractBuild$AbstractBuildExecution.run(AbstractBuild.java:521)
	at hudson.model.Run.execute(Run.java:1894)
	at PluginClassLoader for matrix-project//hudson.matrix.MatrixRun.run(MatrixRun.java:153)
	at hudson.model.ResourceController.execute(ResourceController.java:101)
	at hudson.model.Executor.run(Executor.java:446)
Caused by: hudson.plugins.git.GitException: Command "git fetch --no-tags --force --progress -- git@github.com:nodejs/node.git +refs/heads/*:refs/remotes/origin/* +refs/pull/54289/head:refs/remotes/origin/_jenkins_local_branch" returned status code 128:
stdout: 
stderr: ssh: Could not resolve hostname github.com: Name or service not known
fatal: Could not read from remote repository.

@avivkeller

avivkeller commented Aug 16, 2024 •

Copy link
Copy Markdown
Member Author

I have no idea what keeps causing https://ci.nodejs.org/job/node-test-commit-linux-containered/nodes=ubuntu2204_sharedlibs_openssl30_x64/45342/testReport/junit/(root)/parallel/test_internal_util_colors, I can't reproduce locally.

However, it's only (some of) the linked tests.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@cjihrig

cjihrig commented Aug 22, 2024

Copy link
Copy Markdown
Contributor

I have no idea what keeps causing

Maybe try changing the test to use spawnSync() (there is no real benefit to using spawn() in this case) and asserting that stderr is empty. If the assertion fails, it should show what happened. You can also assert on the process exit code more easily that way.

@avivkeller

Copy link
Copy Markdown
Member Author

I have no idea what keeps causing

Maybe try changing the test to use spawnSync() (there is no real benefit to using spawn() in this case) and asserting that stderr is empty. If the assertion fails, it should show what happened. You can also assert on the process exit code more easily that way.

I changed it to spawnPromisified, which (IIUC) acts similar, but has a easily-accessible code return value.

@avivkeller avivkeller changed the title util: allow colors in non-tty if FORCE_COLORS util: allow colors in non-tty if FORCE_COLOR Aug 23, 2024
@avivkeller avivkeller closed this Sep 23, 2024
@avivkeller

Copy link
Copy Markdown
Member Author

There are a few CI failures, and I have a lot of open PRs. For the best interest of everyone, someone else can take on this work

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

Labels

needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

assert colors don’t match test colors

6 participants