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

testing: use a tighter check if bash is available #7520

Merged
merged 1 commit into from
Jul 20, 2020

Conversation

bluetech
Copy link
Member

Fixes #7518.

This fixes CI on Windows since GitHub Actions started installing WSL on their images which apparently installs some wrapper bash which does not run actual bash.

testing/test_parseopt.py Outdated Show resolved Hide resolved
testing/test_parseopt.py Outdated Show resolved Hide resolved
This fixes CI on Windows since GitHub Actions started installing WSL on
their images which apparently installs some wrapper `bash` which does
not run actual bash.
@The-Compiler
Copy link
Member

The diff looks good, but I'm a bit confused about the CI output now:

testing\test_parseopt.py ............................                    [ 15%]

that looks like those tests are actually passing now, rather than being skipped? 🤔

@bluetech
Copy link
Member Author

The skip in testing/test_parseopt appears at a later line: https://github.com/pytest-dev/pytest/pull/7520/checks?check_run_id=890089914#step:7:115

Copy link
Member

@The-Compiler The-Compiler left a comment

Choose a reason for hiding this comment

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

Ah, I missed the second line mentioning the same file! Seems fine then, merging this. Thanks!

@The-Compiler The-Compiler merged commit 41d211c into pytest-dev:master Jul 20, 2020
@The-Compiler The-Compiler mentioned this pull request Jul 20, 2020
2 tasks
["bash", "--version"],
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
universal_newlines=True,
Copy link
Member

Choose a reason for hiding this comment

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

check=True here?

bash_version = subprocess.run(
["bash", "--version"],
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
Copy link
Member

@graingert graingert Jul 20, 2020

Choose a reason for hiding this comment

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

you only look at the .stdout, so I think it's cleaner with stderr=subprocess.DEVNULL ?

@The-Compiler
Copy link
Member

Sorry for the fast merge, I thought it'd be good to unblock the CI before more people start running into issues with their PRs. I'd be happy to review a follow-up PR (and also wait a bit before merging there) - agreed with check=True being a good idea (though the exception handler should then also handle subprocess.CalledProcessError. Not sure about stderr=subprocess.DEVNULL, I guess stderr=None (i.e. the default) would help debugging issues if there actually was something printed to stderr by the process.

@graingert
Copy link
Member

No worries, I only got the email notification when the merge happened.

@bluetech bluetech deleted the win-bash branch July 23, 2020 10:58
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.

Tests on Windows started failing since WSL was made available on GitHub Actions
3 participants