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

Missing interpreters now error the session on CI by default #567

Merged
merged 7 commits into from
Feb 13, 2022
Merged

Missing interpreters now error the session on CI by default #567

merged 7 commits into from
Feb 13, 2022

Conversation

FollowTheProcess
Copy link
Collaborator

Closes #340

This PR adds a check to determine if nox is running on CI when evaluating sessions with missing interpreters so that such sessions fail by default when being run on CI providers.

This has the desired effect:

Skipped locally showing CI warning

image

CI=true causes an error

image

Note non-zero exit code

--error-on-missing-interpreters for reference

image

I have some questions I'd like people's input on:

  • Should this be documented or just rely on the warning message?
  • The warning message is kinda long, any other suggestions that get the point across? Maybe just say it will fail in CI and leave it there without mentioning --error-on-missing-interpreters? Given that it will show any time a session is skipped a long message could get annoying.
  • Currently we just check for CI existing in os.environ should it explicitly check for true?

@DiddiLeija
Copy link
Collaborator

Should this be documented or just rely on the warning message?

I think some documentation is OK. Maybe a little note at https://nox.thea.codes/en/latest/tutorial.html#testing-against-different-and-multiple-pythons?

Maybe just say it will fail in CI and leave it there without mentioning --error-on-missing-interpreters?

If that's the case, I would prefer some documentation, to say what this warning won't say?

Copy link
Collaborator

@DiddiLeija DiddiLeija left a comment

Choose a reason for hiding this comment

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

Looks good for me now :)

@FollowTheProcess
Copy link
Collaborator Author

@henryiii Does this work for you? Since #340 had quite a lot of discussion around this I'd like to get lots of eyes on this to make sure everyone is happy 🙂

@cjolowicz
Copy link
Collaborator

Can we make it so CI determines just the default for the option?

This has two advantages IMO:

  1. There is a single source of truth for the setting.
  2. You can override the default in CI using --no-error-on-missing-interpreters. IOW overriding works both ways.

@FollowTheProcess
Copy link
Collaborator Author

Can we make it so CI determines just the default for the option?

Sure, that's a good idea, let me play around some more and get back to you 👍🏻

@FollowTheProcess
Copy link
Collaborator Author

Huh, thought I had it working... standby 😂

@FollowTheProcess
Copy link
Collaborator Author

Okay, think I've found a nice way of doing this. Behaviour now:

Skips show warning message
image

CI turns warning into an error
image

Can override the error with --no-error-on-missing-interpreters
image

Copy link
Collaborator

@cjolowicz cjolowicz left a comment

Choose a reason for hiding this comment

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

Thanks! Some minor suggestions inline.

nox/_option_set.py Show resolved Hide resolved
nox/_option_set.py Show resolved Hide resolved
nox/_options.py Outdated Show resolved Hide resolved
tests/test_sessions.py Outdated Show resolved Hide resolved
@FollowTheProcess
Copy link
Collaborator Author

Cool! Thanks for the review, should be good to go now if everyone's happy 🙂

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

Successfully merging this pull request may close these issues.

Change default behavior for --error-on-missing-interpreters
3 participants