Skip to content

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

Merged
cjolowicz merged 7 commits intowntrblm:mainfrom
FollowTheProcess:missing-interpreters-ci
Feb 13, 2022
Merged

Missing interpreters now error the session on CI by default#567
cjolowicz merged 7 commits intowntrblm:mainfrom
FollowTheProcess:missing-interpreters-ci

Conversation

@FollowTheProcess
Copy link
Copy Markdown
Collaborator

@FollowTheProcess FollowTheProcess commented Feb 8, 2022

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
Copy Markdown
Collaborator

DiddiLeija commented Feb 8, 2022

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?

DiddiLeija
DiddiLeija approved these changes Feb 8, 2022
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Collaborator Author

FollowTheProcess commented Feb 12, 2022

@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
Copy Markdown
Collaborator

cjolowicz commented Feb 12, 2022

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
Copy Markdown
Collaborator Author

FollowTheProcess commented Feb 12, 2022

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
Copy Markdown
Collaborator Author

FollowTheProcess commented Feb 12, 2022

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

@FollowTheProcess
Copy link
Copy Markdown
Collaborator Author

FollowTheProcess commented Feb 12, 2022

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

cjolowicz
cjolowicz reviewed Feb 13, 2022
View reviewed changes
Copy link
Copy Markdown
Collaborator

@cjolowicz cjolowicz left a comment •
edited
Loading

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.

Comment thread nox/_option_set.py
Comment thread nox/_option_set.py
Comment thread nox/_options.py Outdated
Comment thread tests/test_sessions.py Outdated
@FollowTheProcess
Copy link
Copy Markdown
Collaborator Author

FollowTheProcess commented Feb 13, 2022

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

@cjolowicz cjolowicz merged commit 538aa6c into wntrblm:main Feb 13, 2022
@FollowTheProcess FollowTheProcess deleted the missing-interpreters-ci branch February 13, 2022 18:17
@VachaShah VachaShah mentioned this pull request Aug 11, 2022
Closed
@pradyunsg pradyunsg mentioned this pull request Sep 4, 2022
Merged
@johnthagen johnthagen mentioned this pull request Sep 6, 2022
Closed
@Winand Winand mentioned this pull request Feb 5, 2023
Open
@johnthagen johnthagen mentioned this pull request May 4, 2023
Open
@samypr100 samypr100 mentioned this pull request Jul 12, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cjolowicz cjolowicz cjolowicz approved these changes

@DiddiLeija DiddiLeija DiddiLeija approved these changes

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

3 participants

@FollowTheProcess @DiddiLeija @cjolowicz