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

Improve reliability of NO_COLOR tests. #3188

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

dag-erling
Copy link

Use env to ensure that NO_COLOR is set to the desired value (or unset) regardless of any initialization that may occur when script forks a shell to run jq in.

emanuele6
emanuele6 previously approved these changes Oct 4, 2024
@emanuele6 emanuele6 dismissed their stale review October 4, 2024 12:01

env -u is non-standard

@dag-erling
Copy link
Author

Do you target any platforms where it's not supported?

I suppose I can remove the -u and revert the removal of the unset line and we'll be no worse off than before. I don't dare try env -i (which is portable) because there might be circumstances where we need PATH etc. to be set up just right to run the tests correctly. Another option is to wrap the command in sh -c "..." so we can set and unset variables without relying on env (e.g. faketty sh -c "unset NO_COLOR; $JQ_NO_B -n ." and faketty sh -c "NO_COLOR=1 $JQ_NO_B -n ."). What do you think?

(The problem I'm trying to fix is that the NO_COLOR=1 case is failing on my machine because NO_COLOR is not being passed through faketty and I can't for the life of me figure out why or reproduce it outside of the context of shtest.)

@emanuele6
Copy link
Member

emanuele6 commented Oct 4, 2024

Do you target any platforms where it's not supported?

I don't know about others, but one of the sytems I usually use to test things is my sdf.org shell account that runs a ~15yo NetBSD, and env there doesn't have it.

I suppose I can remove the -u and revert the removal of the unset line and we'll be no worse off than before. I don't dare try env -i (which is portable) because there might be circumstances where we need PATH etc. to be set up just right to run the tests correctly. Another option is to wrap the command in sh -c "..." so we can set and unset variables without relying on env (e.g. faketty sh -c "unset NO_COLOR; $JQ_NO_B -n ." and faketty sh -c "NO_COLOR=1 $JQ_NO_B -n ."). What do you think?

You can't exactly do faketty sh -c '': look at the faketty() { script -qec "$*" /dev/null; } implementation, but you can probably do something like that tweaking the implementations a bit.

(The problem I'm trying to fix is that the NO_COLOR=1 case is failing on my machine because NO_COLOR is not being passed through faketty and I can't for the life of me figure out why or reproduce it outside of the context of shtest.)

This test does not look very robust, I am also noticing that it does not set SHELL to /bin/sh before invoking script.
At least the util-linux implementation of script invokes $SHELL, not /bin/sh, to run the specified command, so it will be invoking the login shell of the current user that may not be an sh shell (e.g. tcsh, zsh, fish) or may be /bin/false or something else. That should also be fixed.

Use `env` to ensure that `NO_COLOR` is set to the desired value
regardless of any initialization that may occur when `script`
forks a shell to run jq in.

Note that `env -u` is not portable, so there is no good solution
for the first test case.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants