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

Improve stepwise to not forget failed tests #13122

Merged
merged 7 commits into from
Jan 17, 2025

Conversation

nicoddemus
Copy link
Member

@nicoddemus nicoddemus commented Jan 10, 2025

Now --stepwise will remember the last failed test, even if the previous pytest invocations did not receive --stepwise.

Previously it would always clear the cache if not active, which hinders certain interactive workflows, which is the prime use case for the flag.

@psf-chronographer psf-chronographer bot added the bot:chronographer:provided (automation) changelog entry is part of PR label Jan 10, 2025
@nicoddemus nicoddemus force-pushed the stepwise-clear-cache branch from b261556 to d7dfd4a Compare January 10, 2025 15:40
@nicoddemus nicoddemus force-pushed the stepwise-clear-cache branch 3 times, most recently from 96c3adb to 95d2390 Compare January 10, 2025 21:57
@The-Compiler
Copy link
Member

while handy to have it remembered within a "working session", would it be unexpected to have it remembered across such sessions?

I'm worried about this too. Imagine someone:

  • Reads about --stepwise being a thing and gives it a try
  • Uses pytest normally for the next couple of months
  • Now actually wants to use --stepwise after a new Python version came out and broke something

If you've long forgotten about your previous session, that'll be quite surprising...

I don't really have a solution unfortunately. A couple of ideas:

  • Add an argument (or --stepwise=keep or whatever) to opt into keeping it, either when originally using --stepwise, or in the subsequent call without it
  • Add some sort of option for a stepwise max age (say, a week by default?) after which it's discarded
  • Invalidate the cache as soon as new tests are added (not sure what happens currently)
  • Keep the behavior, but make it clearer in the UI: Store a timestamp, and instead of stepwise: skipping 1 already passed items., display something like stepwise: skipping 1 already passed items (cache from 2025-01-10, use --stepwise-reset to discard)

I don't really like the first three as they all come with their own downsides, but maybe the fourth one would at least partially help?

@nicoddemus
Copy link
Member Author

@The-Compiler thanks for the consideration, indeed now that I think about it more it might be surprising unless the user is aware of the change.

Invalidate the cache as soon as new tests are added (not sure what happens currently)
Keep the behavior, but make it clearer in the UI

I think those two are actually reasonable, and they even make sense to be implemented together. I will take a stab at them.

Thanks!

@nicoddemus nicoddemus added this to the 8.4 milestone Jan 12, 2025
src/_pytest/stepwise.py Outdated Show resolved Hide resolved
Copy link
Member

@The-Compiler The-Compiler left a comment

Choose a reason for hiding this comment

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

Amazing work, thanks! Left a couple comments, mainly small nitpicks.

src/_pytest/stepwise.py Outdated Show resolved Hide resolved
src/_pytest/stepwise.py Outdated Show resolved Hide resolved
src/_pytest/stepwise.py Outdated Show resolved Hide resolved
src/_pytest/stepwise.py Outdated Show resolved Hide resolved
src/_pytest/stepwise.py Outdated Show resolved Hide resolved
testing/test_stepwise.py Show resolved Hide resolved
src/_pytest/stepwise.py Show resolved Hide resolved
Now `--stepwise` will remember the last failed test, even if the previous pytest invocations did not pass `--stepwise`.

Previously it would always clear the cache if not active, which hinders certain interactive workflows, which is the prime use cases for the flag.
@nicoddemus nicoddemus force-pushed the stepwise-clear-cache branch from cdf06c3 to 1f7a151 Compare January 14, 2025 09:41
@nicoddemus
Copy link
Member Author

@The-Compiler ready for another review. 👍

Copy link
Member

@The-Compiler The-Compiler left a comment

Choose a reason for hiding this comment

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

Sorry for the delay! Wanted to play around with this a bit rather than just looking at the diff, and didn't get around to it yet.

One last thing I noticed regarding the output, and then I think this is good to go!

else:
self.report_status = f"skipping {failed_index} already passed items."
self.report_status.append(
f"skipping {failed_index} already passed items (cache from {self.cached_info.last_cache_date},"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
f"skipping {failed_index} already passed items (cache from {self.cached_info.last_cache_date},"
f"skipping {failed_index} already passed items (cache from {self.cached_info.last_cache_date:%c},"

maybe, where %c is "Locale’s appropriate date and time representation."?

Without this, it gets displayed as e.g. cache from 2025-01-15 14:25:45.458813 which seems a bit verbose, and the fractional seconds don't really add any useful information.

Copy link
Member Author

@nicoddemus nicoddemus Jan 15, 2025

Choose a reason for hiding this comment

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

I did try that initially, but to me it showed up as the default locale (C): IIUC we need to call locale.setlocale at some point explicitly for %c to work properly, otherwise it will always use the C locale (and we cannot do that because this is a global setting, which pytest definitely should not touch IMO).

But I might be wrong, can you test that in your machine?

If %c does not work, we might explicitly use %YY-%M-%d %h:%m:%s to get rid of the fractional seconds, which I agree is not useful.

Copy link
Member

@The-Compiler The-Compiler Jan 15, 2025

Choose a reason for hiding this comment

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

Oh wow, looks like it! My locale setup is a bit weird at the moment anyways (experimented with en_DK to get proper dates, but turns out it's not supported everywhere so now I get a mess of formats...). Because of that, I expected that to be an issue on my end.

What do you think about making a timedelta to now and then just formatting that in instead? That would then show up as 0:00:10 ago or 42 days, 1:23:45 ago which seems a much nicer format for the information we actually want to convey here.

edit: Whoops, I completely neglected that this would show microseconds too... I suppose that could be fixed with a datetime.timedelta(seconds=int(td.total_seconds())) (with td being the original delta).

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea about timedelta, will do.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

I considered only showing that information if the cache was "old", but then what could be considered an "old" cache? 1 hr? 1 day? In the ended I kept it simple and always show the cache age for now.

Copy link
Member

Choose a reason for hiding this comment

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

Hehe, I have been thinking about that as well, and came to the same conclusion as you did 😅

Copy link
Member

@The-Compiler The-Compiler left a comment

Choose a reason for hiding this comment

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

Thanks for sticking with this, looks great! Will merge in a day or two if nobody else wants to take a look.

@The-Compiler The-Compiler merged commit fc56ae3 into pytest-dev:main Jan 17, 2025
28 checks passed
@The-Compiler
Copy link
Member

Thanks again! ✨

@nicoddemus nicoddemus deleted the stepwise-clear-cache branch January 20, 2025 15:25
@nicoddemus
Copy link
Member Author

Thanks @The-Compiler for merging! However that would be better as a squash probably. 👍

@The-Compiler
Copy link
Member

I did consider that and decide against it, given that there were significant changes/amendments in the history of this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot:chronographer:provided (automation) changelog entry is part of PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants