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

Make cache plugin cumulative #2621

Merged
merged 3 commits into from Jul 28, 2017

Conversation

Projects
None yet
4 participants
@nicoddemus
Member

nicoddemus commented Jul 27, 2017

When introducing a delicate change to a large code base (2000+ tests), I often run all tests (using xdist) to see what has been affected, then ooops, 400 broken tests.

I would like to go fixing module by module to get everything green again.

What happens currently is this:

  1. Run all tests: pytest tests. BOOM, 400 failures. Oh boy.
  2. Run tests from the first module with failures: pytest tests/core --lf. Fix all failures, get a green run.
  3. Run tests from the next module with failures: pytest tests/controller --lf. At this point the cache plugin forgets the previous failures, running all tests found in test/controller again, regardless if they have failed before or not.

Because of this, I often find myself copying/pasting the list of failed modules somewhere so I can run them selectively, instead of relying on the caching mechanism.

The workflow I want:

  1. Run all tests: pytest tests. BOOM, 400 failures. Oh boy.
  2. Run tests from the first module with failures: pytest tests/core --lf. Fix all failures, get a green run.
  3. Run tests from the next module with failures: pytest tests/controller --lf. Fix all failures, get a green run.
  4. Repeat until all modules are fixed.

This PR attempts to fix this by making the cache plugin always remember which tests failed at a certain point, discarding a test failure only when it sees that test passing again.

This is still a WIP because it needs docs/changelog/etc plus I wanted to gather feedback.

@coveralls

This comment has been minimized.

coveralls commented Jul 27, 2017

Coverage Status

Coverage decreased (-0.003%) to 91.914% when pulling 0ae593a on nicoddemus:cumulative-cache into 309152d on pytest-dev:features.

@RonnyPfannschmidt

well done 👍

@The-Compiler

This comment has been minimized.

Member

The-Compiler commented Jul 27, 2017

Can't review the implementation right now, but the idea sounds great!

@nicoddemus

This comment has been minimized.

Member

nicoddemus commented Jul 27, 2017

Made some new changes and added the changelog entry.

@nicoddemus nicoddemus changed the title from Make cache plugin cumulative (WIP) to Make cache plugin cumulative Jul 27, 2017

def pytest_report_header(self):
if self.active:
if not self.lastfailed:
mode = "run all (no recorded failures)"
else:
mode = "rerun last %d failures%s" % (

This comment has been minimized.

@nicoddemus

nicoddemus Jul 27, 2017

Member

Removed this number of failures because it might be incorrect (before and after this patch): we don't know which tests we will actually run because that's decided after collection only. Because of this I decided to remove the promise that we will run X failures at this point.

This comment has been minimized.

@nicoddemus

nicoddemus Jul 27, 2017

Member

This problem is fixed by #2624

" first" if self.config.getvalue("failedfirst") else "")
return "run-last-failure: %s" % mode
def pytest_runtest_logreport(self, report):
if report.failed and "xfail" not in report.keywords:
if report.passed and report.when == 'call':

This comment has been minimized.

@nicoddemus

nicoddemus Jul 27, 2017

Member

Not sure why we would make a special case about xfail tests here... decided to simplify things, but would like a second set of eyes here.

This comment has been minimized.

@RonnyPfannschmidt

RonnyPfannschmidt Jul 27, 2017

Member

xfail is ok to fail so its not really part of "last failed tests"

This comment has been minimized.

@nicoddemus

nicoddemus Jul 27, 2017

Member

OK, will update it, thanks

This comment has been minimized.

@nicoddemus

nicoddemus Jul 27, 2017

Member

I added two new tests specific to deal with xfail, and this logic did not actually need any change in the end. 😁

@@ -147,11 +142,11 @@ def pytest_collection_modifyitems(self, session, config, items):
previously_failed.append(item)
else:
previously_passed.append(item)
if not previously_failed and previously_passed:

This comment has been minimized.

@nicoddemus

nicoddemus Jul 27, 2017

Member

I didn't quite understand why previously_passed was being considered here for this condition... to me it makes sense to skip deselecting items if we didn't find any previous failures in the current collection.

This comment has been minimized.

@RonnyPfannschmidt

RonnyPfannschmidt Jul 27, 2017

Member

when we have failed tests that are outside of the of the selection thats currently being executed, that happens

This comment has been minimized.

@nicoddemus

nicoddemus Jul 27, 2017

Member

Sorry, what happens?

This comment has been minimized.

@RonnyPfannschmidt

RonnyPfannschmidt Jul 27, 2017

Member

if test_a.py::test_a is failed and you run pytest test_b.py --lf then you shouldn't remove passed tests from the test set, that way you can run in lf mode and work subsets of the failure set until they pass

This comment has been minimized.

@nicoddemus

nicoddemus Jul 27, 2017

Member

I see, but unless I'm mistaken that is covered by the new test I added right?

I changed the condition to if not previously_failed: return, IOW don't skip anything if no collected item is part of the previous failures set.

So running pytest test_b.py --lf will only collect test_b.py::* tests, which means previously_failed will be empty and nothing will be skipped. At this point, if all tests from test_b.py pass, we don't lose the fact that test_a.py::test_a failed at some point in the past. If we execute pytest test_a.py --lf, now only test_a.py::test_a will execute, which is the point I'm trying to accomplish here with this PR.

This comment has been minimized.

@RonnyPfannschmidt

RonnyPfannschmidt Jul 27, 2017

Member

sounds correct

prev_failed = config.cache.get("cache/lastfailed", None) is not None
if (session.testscollected and prev_failed) or self.lastfailed:
saved_lastfailed = config.cache.get("cache/lastfailed", {})

This comment has been minimized.

@nicoddemus

nicoddemus Jul 27, 2017

Member

Now that we always have self.last_failed, write it back if it differs from the "default". Again I would appreciate a second set of eyes here.

This comment has been minimized.

@RonnyPfannschmidt

RonnyPfannschmidt Jul 27, 2017

Member

last failed already supported correct updating if it was enabled, making it transient means the updating code has to change

This comment has been minimized.

@nicoddemus

nicoddemus Jul 27, 2017

Member

Not sure what you mean, could you clarify? Also not sure if it was just a general comment or a request for change.

This comment has been minimized.

@RonnyPfannschmidt

RonnyPfannschmidt Jul 27, 2017

Member

general comment, last failed in last failed mode supports working correctly when the test selection doesn't match the failure selection

This comment has been minimized.

@nicoddemus

nicoddemus Jul 27, 2017

Member

OK thanks

@coveralls

This comment has been minimized.

coveralls commented Jul 27, 2017

Coverage Status

Coverage decreased (-0.003%) to 91.881% when pulling 3e5a5b5 on nicoddemus:cumulative-cache into ddf1751 on pytest-dev:features.

@RonnyPfannschmidt

the xfail related behaviour clearly needs a closer look

this part of the codebase begs for unittests ^^

@coveralls

This comment has been minimized.

coveralls commented Jul 27, 2017

Coverage Status

Coverage decreased (-0.003%) to 91.881% when pulling d6ce547 on nicoddemus:cumulative-cache into ddf1751 on pytest-dev:features.

@nicoddemus

This comment has been minimized.

Member

nicoddemus commented Jul 27, 2017

Rebased on latest features.

@coveralls

This comment has been minimized.

coveralls commented Jul 27, 2017

Coverage Status

Coverage decreased (-0.003%) to 91.883% when pulling 22212c4 on nicoddemus:cumulative-cache into e97fd5e on pytest-dev:features.

@nicoddemus

This comment has been minimized.

Member

nicoddemus commented Jul 27, 2017

Ready to be reviewed by @The-Compiler. 👍

@The-Compiler

This comment has been minimized.

Member

The-Compiler commented Jul 27, 2017

Nope, sorry - exams coming up, not enough brain power left for code 😉 Either someone merges this, or I'll come back to it, but probably not before September.

@nicoddemus

This comment has been minimized.

Member

nicoddemus commented Jul 27, 2017

I'll come back to it, but probably not before September.

No worries, thanks for validating the general idea anyway! Good studies!

xfail and skipped tests are removed from the "last-failed" cache
This accommodates the case where a failing test is marked as
skipped/failed later
@coveralls

This comment has been minimized.

coveralls commented Jul 27, 2017

Coverage Status

Coverage decreased (-0.003%) to 91.883% when pulling eb1bd34 on nicoddemus:cumulative-cache into e97fd5e on pytest-dev:features.

@nicoddemus

This comment has been minimized.

Member

nicoddemus commented Jul 28, 2017

Anything else here @RonnyPfannschmidt? Otherwise I think we can go ahead and merge this.

@RonnyPfannschmidt RonnyPfannschmidt merged commit 1712196 into pytest-dev:features Jul 28, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nicoddemus

This comment has been minimized.

Member

nicoddemus commented Jul 28, 2017

Thanks! 👍

@nicoddemus nicoddemus deleted the nicoddemus:cumulative-cache branch Jul 28, 2017

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