Make cache plugin cumulative by nicoddemus · Pull Request #2621 · pytest-dev/pytest

nicoddemus · 2017-07-27T00:29:09Z

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:

Run all tests: pytest tests. BOOM, 400 failures. Oh boy.
Run tests from the first module with failures: pytest tests/core --lf. Fix all failures, get a green run.
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:

Run all tests: pytest tests. BOOM, 400 failures. Oh boy.
Run tests from the first module with failures: pytest tests/core --lf. Fix all failures, get a green run.
Run tests from the next module with failures: pytest tests/controller --lf. Fix all failures, get a green run.
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 · 2017-07-27T01:01:05Z

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 · 2017-07-27T08:01:02Z

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

nicoddemus · 2017-07-27T13:01:55Z

Made some new changes and added the changelog entry.

nicoddemus · 2017-07-27T13:03:14Z

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


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 problem is fixed by #2624

nicoddemus · 2017-07-27T13:03:39Z


    def pytest_runtest_logreport(self, report):
-        if report.failed and "xfail" not in report.keywords:
+        if report.passed and report.when == 'call':


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.

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

OK, will update it, thanks

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

nicoddemus · 2017-07-27T13:05:16Z

                    previously_failed.append(item)
                else:
                    previously_passed.append(item)
-            if not previously_failed and previously_passed:


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.

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

Sorry, what happens?

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

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.

sounds correct

nicoddemus · 2017-07-27T13:05:54Z

-        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", {})


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.

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

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

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

coveralls · 2017-07-27T13:30:53Z

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 · 2017-07-27T14:48:59Z

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

nicoddemus · 2017-07-27T17:41:13Z

Rebased on latest features.

coveralls · 2017-07-27T18:06:44Z

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

nicoddemus · 2017-07-27T20:17:00Z

Ready to be reviewed by @The-Compiler. 👍

The-Compiler · 2017-07-27T20:20:33Z

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 · 2017-07-27T20:26:59Z

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

coveralls · 2017-07-27T22:17:00Z

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

nicoddemus · 2017-07-28T10:43:34Z

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

nicoddemus · 2017-07-28T11:51:34Z

Thanks! 👍

nicoddemus requested review from RonnyPfannschmidt and The-Compiler July 27, 2017 00:29

RonnyPfannschmidt approved these changes Jul 27, 2017

View reviewed changes

nicoddemus force-pushed the cumulative-cache branch from 0ae593a to 3e5a5b5 Compare July 27, 2017 13:01

nicoddemus changed the title ~~Make cache plugin cumulative (WIP)~~ Make cache plugin cumulative Jul 27, 2017

nicoddemus commented Jul 27, 2017

View reviewed changes

nicoddemus mentioned this pull request Jul 27, 2017

New report hook to display messages after collection is finished #2622

Closed

RonnyPfannschmidt requested changes Jul 27, 2017

View reviewed changes

RonnyPfannschmidt approved these changes Jul 27, 2017

View reviewed changes

nicoddemus added 2 commits July 27, 2017 14:40

Make cache plugin always remember failed tests

62810f6

Add xfail specific tests

22212c4

nicoddemus force-pushed the cumulative-cache branch from d6ce547 to 22212c4 Compare July 27, 2017 17:40

nicoddemus mentioned this pull request Jul 27, 2017

Fix --last-failed reported items in terminal #2624

Merged

xfail and skipped tests are removed from the "last-failed" cache

eb1bd34

This accommodates the case where a failing test is marked as skipped/failed later

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

nicoddemus deleted the cumulative-cache branch July 28, 2017 11:51

Uh oh!

Conversation

nicoddemus commented Jul 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Jul 27, 2017

Uh oh!

RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

Uh oh!

The-Compiler commented Jul 27, 2017

Uh oh!

nicoddemus commented Jul 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicoddemus Jul 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Jul 27, 2017

Uh oh!

RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

Uh oh!

coveralls commented Jul 27, 2017

Uh oh!

nicoddemus commented Jul 27, 2017

Uh oh!

coveralls commented Jul 27, 2017

Uh oh!

nicoddemus commented Jul 27, 2017

Uh oh!

The-Compiler commented Jul 27, 2017

Uh oh!

nicoddemus commented Jul 27, 2017

Uh oh!

coveralls commented Jul 27, 2017

Uh oh!

nicoddemus commented Jul 28, 2017

Uh oh!

nicoddemus commented Jul 28, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

nicoddemus commented Jul 27, 2017 •

edited

Loading

nicoddemus Jul 27, 2017 •

edited

Loading