Run tests in development without subprocesses.

[ryneeverett]

This runs notably faster even with the small number of tests we
currently have and can make debugging easier in certain situations.

However I'd suggest we continue using subprocesses in CI for better
isolation. This will ensure that we don't accidentally introduce
inter-test state dependence which in my experience is a common cause of
false negative situations where tests don't actually test what they're
supposed to.

[Ekleog]

LGTM :) let's merge after a bit if no one objects to the change

[ryneeverett]

Pushed another commit adding support for pdb which I'd forgotten was my motivation in the first place.

[auouymous]

This feels wrong to me. The tests might pass locally and then fail after pushing? It would also be possible for that inter-test state to cause tests to fail locally and waste developer time trying to fix something that isn't broken.

[ryneeverett]

This feels wrong to me. The tests might pass locally and then fail after pushing? It would also be possible for that inter-test state to cause tests to fail locally and waste developer time trying to fix something that isn't broken.

I understand where you're coming from, but from my perspective running each test in a separate process also feels wrong. It's unconventional, less performant, and prevents jumping into the code under test with pdb. Slow tests in development and half-broken pdb is also a waste of developer time.

To address your point about the cost of fixing inter-test state bugs, that is the cost of conventional python testing. The difference between the pattern I'm advocating and typical python projects is that we'll know when we have an inter-test state bug rather than being unaware of the false negative.

I'll also note that if there's sufficient resistance to this change I could make subprocess the default and make non-subprocess the optional flag. Then we can change the default in a year or so when the tests take over a minute to run. :)

[auouymous]

Correctness is more important than performance, especially for tests. It should be the default, and it should remain the default even when there are 100x as many tests and it takes over a minute to run them. Someone who chooses performance will hopefully understand that the test results might be incorrect.

[ryneeverett]

Correctness is more important than performance, especially for tests.

Especially for tests? I was under the impression that automated tests are a practical (time-saving) tool to ensure the correctness of the software itself. :)

[ryneeverett]

Another advantage to conventional python testing is that the teardown serves as documentation of state and encourages intentional state structure/design. Currently it appears that all the state -- at least that pertinent to the current tests -- lives in the configuration and log objects. When new state is added that's an opportunity to decide whether it truly warrants a new state object or would be better placed in an existing state object.

[Ekleog]

Random thought: what about making tests do something like this?

1. run all tests with in-process mode
2. display a preliminary result
3. re-run all tests with out-of-process mode, confirming the results

I wonder how bad something like this would interact with some IDE integrations, though, but for manual test running it'd probably be optimal — getting a quick feedback on what's wrong, but if waiting a bit more getting all the results.

This being said, an alternative might just be to finish the “fast” version of the tests with “please re-run with --subprocess for getting the same result as CI”.

Actually… thinking some more about it, I kind of don't feel that bad about tests passing locally but not in CI. Having more tests being run by CI than are run locally is something that's very usual. Here, if we have an easy way of replicating CI behavior locally, yet have tests run fast when they can… I'd say it probably makes sense?

WDYT, @auouymous ?

[ryneeverett]

I hadn't seen the test README before and hadn't realized that by running test.py I wasn't running all the tests -- and neither was CI. I added a commit fixing this and adding a test runner script which makes this feature work with test discovery.

[ryneeverett]

The test runner script will also print a warning when not using the --subprocess flag to address @auouymous' concern that developers will be taken unaware when their PR fails CI.

[kaashif]

I know it's kind of late to weigh in on this, but I think I'm fine with tests being fast locally, even if there might be some chance of bugs. As long as the CI runs the tests completely (and slowly, as a result), and there is a way to have the CI behaviour locally.

[Ekleog]

This statement sounds reasonable to me, but… tests currently take 4 seconds on CI (https://github.com/rss2email/rss2email/runs/2144629602), is that really something we want to get down from at the expense of correctness? If we were in the 30+ seconds bucket I'd totally understand, but for <10 seconds I feel like the default should be correctness, and people who want more speed should be the ones who have to use a special flag

[kaashif]

I agree, I'm not in any rush to approve or merge this, I can't see the benefit. But if we did have this, it wouldn't actually compromise the quality of the software, as long as the full tests did have to run before merging changes.

[Ekleog]

Seeing how the discussion stabilized, let's close this, as getting tests a tad faster than their current <5s is probably not worth adding some even low likelihood of breakage