#7938 - [Plugin: Stepwise][Enhancements] Refactoring, smarter registration & --sw-skip functionality

[symonk]

Hi team, hope you are all keeping well. While trying to advance my knowledge of the project I have been understanding plugins a little bit. Here is a small PR to tidy up some of the stepwise plugin code for consistency. Some of the most important changes are:

• Stop registering the plugin if stepwise itself is not active (rather than these constant is_active checks inside hooks).
• Adding a --sw-skip shorthand option to mirror that of --sw itself
• Added a pytest_sessionfinish hook to stepwise (this is because --cache-clear was always outputting the [] now, I wanted to keep --cache-clear accurate in such circumstances and claim the cache is 'empty' ? However (imo) this should be handled in pytest_unconfigure and when --sw is not present, stepwise is always writing the [] upfront. - need advice here

I moved some strings out into more constants/globals - curious on your thoughts on that & secondly I think we are permitted f string use now (that 3.5+ is no longer officially supported? - I think that is the case anyway).

closes #7938

As always - thanks for all the great contributions to this open source project everyone! I appreciate your own time to review this and welcome any feedback.

[bluetech]

Thanks @symonk.

👍 on the --sw-skip shorthand from me.

Regarding the plugin registration change, I think the is_active stuff was done in order to encapsulate the

            # Clear the list of failing tests if the plugin is not active.
            self.config.cache.set("cache/stepwise", [])

part of the plugin inside the plugin. Even though the is_active checks are ugly, I think it does make sense to keep it encapsulated. This way the plugin can be understood on its own, without having to know about the external hook which changes it.

[symonk]

Thanks @symonk.

👍 on the --sw-skip shorthand from me.

Regarding the plugin registration change, I think the is_active stuff was done in order to encapsulate the

            # Clear the list of failing tests if the plugin is not active.
            self.config.cache.set("cache/stepwise", [])

part of the plugin inside the plugin. Even though the is_active checks are ugly, I think it does make sense to keep it encapsulated. This way the plugin can be understood on its own, without having to know about the external hook which changes it.

Thanks @symonk.

👍 on the --sw-skip shorthand from me.

Regarding the plugin registration change, I think the is_active stuff was done in order to encapsulate the

            # Clear the list of failing tests if the plugin is not active.
            self.config.cache.set("cache/stepwise", [])

part of the plugin inside the plugin. Even though the is_active checks are ugly, I think it does make sense to keep it encapsulated. This way the plugin can be understood on its own, without having to know about the external hook which changes it.

I see, it feels a bit weird to register a plugin that is going to perform next to no work except setting stepwise in cache to [], would it not be a (albeit it negligible) performance benefit to avoid doing so under certain circumstances? Save us a few hookimpl invocations etc needlessly, that said I'm still infantly learning how the plugin system works so I will defer to your knowledge on this one

[bluetech]

would it not be a (albeit it negligible) performance benefit to avoid doing so under certain circumstances? Save us a few hookimpl invocations etc needlessly

Yes, definitely. In particular the pytest_runtest_logreport can be called quite a lot (3 times per test) and reducing the number of hook calls of it is beneficial.

---

Looking again, now in my editor, since the entire file if dedicated to just the one plugin, there is not much risk of missing out what's going on as I feared in my comment. And if you keep the comment "# Clear the list of failing tests if the plugin is not active." then it even makes more sense. So I'm good with that part too.

[symonk]

@bluetech thanks for the feedback will address this one after work today

[nicoddemus]

Cool, good work @symonk!

I've left a few comments, take a look and let me know what you think. 👍

[nicoddemus]

This is incorrect, items at this point only contains the failed item and the other items after it (see my suggestion).

[nicoddemus]

Hmmm that no tests caught this is troubling actually, most likely because this only affects the output summary.

[symonk]

whoops my bad, will also add coverage there - yes very troubling :)

[symonk]

@nicoddemus I (hope) my latest commit can cover this scenario, tho if you could have a look that would be appreciated. I plan as a follow up (subsequent ticket/PR) to move away from testdir here and use pytester instead.

[bluetech]

@symonk If you want to test this, you can add a pytest_deselected hookimpl in a conftest.py file in the test, and check that it receives what is expected.

[nicoddemus]

LGTM, thanks @symonk, great work!

(We should squash the changes during merge)

[symonk]

Thanks, @bluetech are you happy with this now? (unable to squash-merge w/o reviews)

[symonk]

Thanks @bluetech

[bluetech]

The ubuntu-pypy3 CI failure unfortunately looks legitimate.

[symonk]

damnit haha, will get to it shortly

[symonk]

@bluetech, @nicoddemus I can only assume the ubuntu-pypy3 is flaky? as it passed this time round :/

[bluetech]

I can only assume the ubuntu-pypy3 is flaky? as it passed this time round :/

Since the failure happened in a relevant test, I suspect it is not flaky. But I suggest we merge and see if it recurs. If it does recur, and we don't find the cause, we might need to revert...

[symonk]

ok sounds good, il squash merge and keep an eye on it in other PRs regarding these tests