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

suggest disabling xdist in internals guide #1122

Closed
wants to merge 3 commits into from

Conversation

dchudz
Copy link
Member

@dchudz dchudz commented Feb 18, 2018

Came up because of: #1093 (comment)

Or alternatively:

I imagine the default -n 2 in tox.ini is mostly for CI? If humans mostly want xdist disabled, I wonder about not specifying a default in tox.ini and then doing whatever we need to for CI to still have -n 2?

@DRMacIver
Copy link
Member

DRMacIver commented Feb 18, 2018

I imagine the default -n 2 in tox.ini is mostly for CI? If humans mostly want xdist disabled, I wonder about not specifying a default in tox.ini and then doing whatever we need to for CI to still have -n 2?

Yeah, I think this is a good idea.

Happy to merge this as is, but would you like to change it to just doing this by default instead?

@dchudz
Copy link
Member Author

dchudz commented Feb 18, 2018

would you like to change it to just doing this by default instead?

Yeah, I'll do that. (Mostly opened this PR as a place for discussion about doing that instead.)

@dchudz
Copy link
Member Author

dchudz commented Feb 18, 2018

That'll also solve the problem where people don't install xdist, get the error about -n not being an option, and can't run tests. (The guide is pretty clear, but I bet it would keep happening anyway.)

@dchudz
Copy link
Member Author

dchudz commented Feb 18, 2018

I'm looking into whether there's a way to have tox default to using -n 2 but not default to that running pytest directly.

If I can't find a way to do that, I think I would need to add -n 2 to all the pytest commands in tox.ini (except the ones that already do -n 0 explicitly), and also in basic-test.sh.

@alexwlchan
Copy link
Contributor

@dchudz Small Internet, huh? 😉

@dchudz
Copy link
Member Author

dchudz commented Feb 19, 2018

Hah, thanks @alexwlchan! (for anyone wondering, I asked the internet how to do this, and Alex came answering...: https://stackoverflow.com/questions/48856608/set-pytest-arg-for-tox-but-not-direct-pytest)

@dchudz
Copy link
Member Author

dchudz commented Feb 19, 2018

Hmm, as an aside, it looks to me like setenv has to go in the [testenv] section, not the [tox] section. So unless I'm confused about its purpose, it looks to me like this bit in [tox] isn't having any effect:

setenv=
  PYTHONWARNINGS={env:PYTHONWARNINGS:error::DeprecationWarning,error::FutureWarning}

(I tried and it seems like tests currently don't object to DeprecationWarning and FutureWarning.)

@dchudz
Copy link
Member Author

dchudz commented Feb 19, 2018

Just pushed the settings change to this PR. I checked that e.g. make check-fast runs with 2 xdist workers, but e.g. HYPOTHESIS_VERBOSITY_LEVEL=debug pytest tests/quality/test_shrink_quality.py -k test_minimize_multiple_elements_in_silly_large_int_range -s runs without xdist (and prints all of the stuff I'd expect).

@dchudz
Copy link
Member Author

dchudz commented Feb 19, 2018

Actually I'll replace this with a new PR with a better title / branch name and without the useless commits.

@Zac-HD
Copy link
Member

Zac-HD commented Feb 19, 2018

FYI the PYTHONWARNINGS bit should use the user's environment if the variable is set; but make those warnings into errors if it isn't.

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.

None yet

4 participants