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

ci: add --color=yes to pytest as needed in github actions #430

Merged
merged 1 commit into from
Aug 27, 2021

Conversation

consideRatio
Copy link
Collaborator

@consideRatio consideRatio commented Aug 26, 2021

GitHub actions doesn't make pytest realize it can emit colors as for example TravisCI did. It is a long standing issue open about this someplace I remember.

@jacobtomlinson
Copy link
Member

🤯 this is amazing! For some reason I assumed the GHA logs were black and white.

I wonder if there is some neater way to do this though? Some environment variable or something that we can set to help pytest figure this out correctly.

@jacobtomlinson
Copy link
Member

We could probably just set PYTEST_ADDOPTS="--color=yes"

@jacobtomlinson
Copy link
Member

It looks like this is how pytest do it themselves.

https://github.com/pytest-dev/pytest/pull/8633/files

@jacobtomlinson
Copy link
Member

Also it looks like @jrbourbeau has done this in setup.cfg over on dask/dask#8090

@consideRatio
Copy link
Collaborator Author

I wonder if there is some neater way to do this though? Some environment variable or something that we can set to help pytest figure this out correctly.

I would find it less clean to put in an environment variable because it would decouple the logic from the location its relevant even though it would avoid repeating the logic.

If it is configured in some repo wide config file, it would be applied systematically no matter what even though it may be some edge case when its better with auto as the default than yes.

In this PR I've opted to not set --color=yes in one location because it would override the default of auto for normal users running tests locally etc with their terminal setups.

I'm happy to adjust this PR to any concrete suggestion getting the job done in another way though!

@jacobtomlinson
Copy link
Member

I would find it less clean to put in an environment variable because it would decouple the logic from the location its relevant even though it would avoid repeating the logic.

I agree with the sentiment here but I think for something as trivial as enabling colours this is fine.

If it is configured in some repo wide config file, it would be applied systematically no matter what even though it may be some edge case when its better with auto as the default than yes.

We would effectively be changing the default from auto to yes. And for the rare edge case situation where this is a problem they would have to set it back to auto. I personally feel it would be better to optimise for the majority.

My preference would probably be to just throw this in setup.cfg and not muddy up all the individual py.test calls.

@consideRatio consideRatio changed the title ci: add --color=yes to pytest as needed in github actions Add --color=yes to pytest as needed in github actions Aug 26, 2021
@consideRatio
Copy link
Collaborator Author

@jacobtomlinson I've updated pyproject.toml to configure pytest. setup.cfg wouldn't work because those files are located under directories for the different python packages while py.test is called from a directory higher up.

@jcrist jcrist merged commit e2ee99b into dask:main Aug 27, 2021
@jcrist
Copy link
Member

jcrist commented Aug 27, 2021

Thanks!

@consideRatio consideRatio changed the title Add --color=yes to pytest as needed in github actions ci: add --color=yes to pytest as needed in github actions Apr 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants