Skip to content

Show plan at start of xdist runs, instead of at end #71

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

Closed
allisonkarlitskaya opened this issue Jul 6, 2023 · 3 comments · Fixed by #76
Closed

Show plan at start of xdist runs, instead of at end #71

allisonkarlitskaya opened this issue Jul 6, 2023 · 3 comments · Fixed by #76

Comments

@allisonkarlitskaya
Copy link

Our CI infra likes to have its plan up front so that it can print a nice progress bar. Currently, when combining pytest-xdist and pytest-tap everything works nicely, except that the plan comes at the end. That's valid TAP, but not so nice for us.

It's possible to use the pytest_xdist_node_collection_finished() hook to write the test plan up front.

@mblayman
Copy link
Member

mblayman commented Jul 6, 2023

Thanks for the report!

I'd imagine that this could be implemented by using the hook you described and doing something similar to https://github.com/python-tap/pytest-tap/blob/main/src/pytest_tap/plugin.py#L74. I'm not sure if that would conflict with the existing hook, but I suspect that it wouldn't because I think set_plan is mostly an idempotent operation.

I don't currently have the time to implement and test this myself, but I would be happy to cut a new release if the feature was implemented in a PR and merged.

@allisonkarlitskaya
Copy link
Author

I took a look at implementing this, but it turned out to be a little bit difficult owing to the fact that we should only set the plan line in "stream" or "combined" modes, and the information on which mode we're in is currently only accessible from the session object, and this object isn't passed in to pytest_xdist_node_collection_finished().

We could read it back from the Tracker, but then we'd probably want to adjust the other places which use this information also to read it from the Tracker, but only in case the plugin is enabled in the first place, which made me want to take a different approach to how state was tracked in the plugin as well... perhaps only instantiating the tracker if the plugin was enabled?

Things spun out of control shortly after that and I basically noticed I was effectively doing a rewrite...

That's here:

https://github.com/allisonkarlitskaya/pytest-tapreporter/blob/main/src/pytest_tapreporter/plugin.py

vs. python-tap, there's the following benefits:

  • no external tap dependency
  • support for yaml output

and the following disadvantages:

  • only supports streaming mode

I feel a bit bad about this because I think it's a bit ridiculous to have two plugins to do the same thing, particularly when the thing in question is so simple... I'd actually be interested in fixing the original issue as reported here in pytest-tap.

I guess my question is: how invasive should the fix be? I feel the reorganization of the plugin to be implemented as a class which only gets instantiated if we enable the plugin via the commandline switch is a good change, but maybe it's too much?

From our side, probably the biggest thing impacting our ability to use pytest-tap (other than this immediate issue about plan line on xdist) is the fact that it depends on a separate module (tap) which isn't packaged in Fedora. Of course, that's the part that implements the extra features (file output), so that's understandable, too. So maybe it does make sense to keep the two code bases separate...

@mblayman
Copy link
Member

I'd be ok with a change to a class-based plugin. I don't really like the module level state that currently exists. IIRC, a class-based plugin wasn't an option at the time of the initial creation of this plugin (or, if it was, I didn't know of those docs).

The challenge though is that I'd still want to use tap.py as the core because that's the place where this project gets all its line parsing logic. Keeping that seems at odds with your goals in Fedora.

If you want, we could get https://tappy.readthedocs.io/en/latest/alternatives.html updated to show your pytest plugin as a zero dependency version. Zero dependency options are a good thing to exist in the world. 😁

mblayman added a commit that referenced this issue Jul 15, 2023
mblayman added a commit that referenced this issue Jul 15, 2023
* Set the plan when using xdist.

Fixes #71

* Credit Allison because this is mostly her idea.
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 a pull request may close this issue.

2 participants