Make cache plugin cumulative #2621
Conversation
|
Can't review the implementation right now, but the idea sounds great! |
nicoddemus
force-pushed
the
cumulative-cache
branch
from
0ae593a
to
3e5a5b5
Compare
|
Made some new changes and added the changelog entry. |
nicoddemus
changed the title
|
|
||
| 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" % ( |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
This problem is fixed by #2624
_pytest/cacheprovider.py
Outdated
| " 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': |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
xfail is ok to fail so its not really part of "last failed tests"
There was a problem hiding this comment.
OK, will update it, thanks
There was a problem hiding this comment.
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: | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
when we have failed tests that are outside of the of the selection thats currently being executed, that happens
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
| 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", {}) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
last failed already supported correct updating if it was enabled, making it transient means the updating code has to change
There was a problem hiding this comment.
Not sure what you mean, could you clarify? Also not sure if it was just a general comment or a request for change.
There was a problem hiding this comment.
general comment, last failed in last failed mode supports working correctly when the test selection doesn't match the failure selection
nicoddemus
force-pushed
the
cumulative-cache
branch
from
d6ce547
to
22212c4
Compare
|
Rebased on latest |
|
Ready to be reviewed by @The-Compiler. 👍 |
|
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. |
No worries, thanks for validating the general idea anyway! Good studies! |
This accommodates the case where a failing test is marked as skipped/failed later
|
Anything else here @RonnyPfannschmidt? Otherwise I think we can go ahead and merge this. |
|
Thanks! 👍 |
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:
pytest tests. BOOM, 400 failures. Oh boy.pytest tests/core --lf. Fix all failures, get a green run.pytest tests/controller --lf. At this point the cache plugin forgets the previous failures, running all tests found intest/controlleragain, 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:
pytest tests. BOOM, 400 failures. Oh boy.pytest tests/core --lf. Fix all failures, get a green run.pytest tests/controller --lf. Fix all failures, get a green run.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.