Skip to content
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

nicoddemus
Copy link
Member

@nicoddemus 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
Copy link

Coverage Status

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

Copy link
Member

@RonnyPfannschmidt RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well done 👍

@The-Compiler
Copy link
Member

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

@nicoddemus
Copy link
Member Author

Made some new changes and added the changelog entry.

@nicoddemus nicoddemus changed the title Make cache plugin cumulative (WIP) 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" % (
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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':
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, will update it, thanks

Copy link
Member Author

@nicoddemus nicoddemus Jul 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, what happens?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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", {})
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK thanks

@coveralls
Copy link

Coverage Status

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

Copy link
Member

@RonnyPfannschmidt RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the xfail related behaviour clearly needs a closer look

this part of the codebase begs for unittests ^^

@coveralls
Copy link

Coverage Status

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

@nicoddemus
Copy link
Member Author

Rebased on latest features.

@coveralls
Copy link

Coverage Status

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

@nicoddemus
Copy link
Member Author

Ready to be reviewed by @The-Compiler. 👍

@The-Compiler
Copy link
Member

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
Copy link
Member Author

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
Copy link

Coverage Status

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

@nicoddemus
Copy link
Member Author

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
@nicoddemus
Copy link
Member Author

Thanks! 👍

@nicoddemus nicoddemus deleted the cumulative-cache branch July 28, 2017 11:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants