don't default to xdist -n2 for pytest

[dchudz]

-n2 is a frustrating default for humans because (among other reasons) xdist doesn't play well with no-capture, so e.g. running a test like HYPOTHESIS_VERBOSITY_LEVEL=debug pytest tests/quality/test_shrink_quality.py -k test_minimize_multiple_elements_in_silly_large_int_range -s (as suggested in the "Playing Around" section of our internals guide. We thought about changing the guide, but really we'd rather change the default.

This change:

• removes -n2 from the [pytest] section of our tox.ini
• adds -n2 back as an argument to pytest in a bunch of places

Alternatives considered attempted:

• tried to use PYTEST_ADDOPTS in the tox environment, but this had some bad consequences and working around them was bad

Replaces #1122.

[Zac-HD]

Looks good to me. Ideally Pytest would provide some way to say "run in multiple jobs if you can", but without that I think disabling xdist by default makes sense.

---

Unrelated, but while you're in the area: you mentioned that the warnings setenv might not work as intended. Can you (a) confirm that it does work, (b) fix it if it doesn't, or (c) open an issue for someone else to do so?

[dchudz]

Sure! Opened an issue: #1124.

I'd have been happy to make the fix (still might be, just not tonight), but fixing that breaks everything, so I just opened an issue.

[dchudz]

Ideally Pytest would provide some way to say "run in multiple jobs if you can" ...

I think that's not what we want, or at least not until/unless xdist can play nicer with -s (no capture). See #1093 (comment).

Sorry I didn't put that context in the original PR before (err, or this one).

[Zac-HD]

Leaving it for later is perfectly fine, and given the size of the task I'd do exactly that too 😄

(maybe the open source fairy will output capturing and add best-effort xdist at the same time?)

[dchudz]

Hey @Zac-HD, sorry to switch this on you, but I realized it would probably be better not to clobber the user's PYTEST_ADDOPTS if they have one.

I think the principle here is: our tox settings should do exactly what they need to for CI by default, and mess with decisions developers made on purpose in their environment as little as possible.

I tested this by checking that with PYTEST_ADDOPTS unset, I get 2 xdist workers, but after export PYTEST_ADDOPTS="-n 0", tox runs without xdist.

[dchudz]

I'll try to look at the CI failures after work - adding [WIP] to the PR title to try to avoid having anyone but me spend time looking at this until I've sorted that out.

[dchudz]

Okay, I just pushed a fix that I think should make things pass now. But I now have no confidence that this is a good change, or that it's even worth any of your time to look at it and think about it. ☹️

Here's what was happening:

Adding PYTEST_ADDOPTS={env:PYTEST_ADDOPTS:"-n 2"} to [testenv] (instead of -n 2 in [pytest]) had the desired effect on the top level of test-running, but the tests in tests/pytest run some tests, and those tests depended on the fact that they weren't run in xdist (b/c they need to capture output), giving me a bunch of failures about not finding output that was expected to be there.

I've addressed this by adding -n0 to those, but that's complicating things a bit -- and the fact that PYTEST_ADDOPTS is set while we run these tests is making something that was already a bit brittle even moreso.

On the other hand, maybe it's an improvement for these tests to be more explicit about the behavior they depend on. (I also added -n0 to some ones that weren't failing, but that don't actually test much if you don't get the output.)

So anyway... 😦 If this doesn't seem like a good plan anymore, I won't be offended or anything.

[DRMacIver]

If this doesn't seem like a good plan anymore, I won't be offended or anything.

Sorry, I hadn't realised this was such a mess or I'd have given you more warning. 😞

I still like the idea, but I'm a bit uncomfortable with the amount of places we now have to put -n0.

I think it might be better to flip the defaults and only turn on -n2 in cases where it's safe and useful to do so.

The big place we would probably benefit from it it is in the somewhat misleadingly named basic-test.sh where the majority of our testing time is spent (basic as in fundamental rather than simple I guess? I can't remember whether that's what I was thinking when I named it or it's just had a hell of a lot of scope creep). Perhaps the thing to do is:

1. Have no global config for xdist.
2. Add -n2 to every pytest line in basic-test.sh where it doesn't break things.
3. Ignore any other tests - they're probably a small test subset that doesn't actually benefit that much from running in parallel.

[dchudz]

That makes lots of sense, thanks! Will do.

I hadn't quite realized everything is quick outside of basic-test.sh.

[DRMacIver]

I hadn't quite realized everything is quick outside of basic-test.sh.

It's not entirely true that everything else is quick - e.g. the django tests and the coverage tests aren't quick, but we don't run those with xdist anyway.

That being said, it might be worth taking a look at "check-quality" and the various pandas tox jobs to see if adding "-n 2" is worth it there (or just add it and don't check, as it'll at least be no worse than status quo there!"). They're on the longer side for our build jobs.

[dchudz]

Hmmmmm...

Slave 'gw0' crashed while running 'tests/nocover/test_large_examples.py::test_can_generate_large_lists_with_min_size'

https://travis-ci.org/HypothesisWorks/hypothesis-python/jobs/344147346

Doesn't seem related to this PR, so I restarted the build. Not certain whether there's any other action we should take about that.

[dchudz]

This should be ready to go if anyone's up for giving me a look and hopefully a 👍 . I've updated the description at the top to better explain what it's about.

[DRMacIver]

I'm not sure defining this here is actually a space saving vs explicitly opting into it in the places needed. It's resulted in an awful lot of -n0 arguments in tests/pytest that seem mostly to just be noise!

One option would be to do something like:

PYTEST_NOXDIST="python -m pytest"
PYTEST="$PYTEST_NOXDIST -n2"

And then use "PYTEST_NOXDIST" in the places that don't need it if those really are the minority (which I guess they are).

[dchudz]

hey @DRMacIver, ugh, I'm sorry about. The -n0s are no longer necessary since I followed your suggestion in #1123 (comment).

I thought I had removed them once they could be removed, but hadn't. 😦

[dchudz]

(they're gone now)

[DRMacIver]

LGTM 🎉

Thanks for doing this! I know the build is um kinda weird and involved, and probably not much fun to work on, so it's doubly appreciated.