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

Handle pytest_warning_captured in workers #341

Closed
nicoddemus opened this issue Sep 24, 2018 · 9 comments
Closed

Handle pytest_warning_captured in workers #341

nicoddemus opened this issue Sep 24, 2018 · 9 comments
Assignees

Comments

@nicoddemus
Copy link
Member

Currently the warnings are no longer being transferred from workers to master.

@nicoddemus nicoddemus self-assigned this Sep 24, 2018
@nicoddemus
Copy link
Member Author

Oh oh. What to do about the item parameter of the pytest_warning_captured hook?

@RonnyPfannschmidt
Copy link
Member

@nicoddemus its a unfixable error as is

@nicoddemus
Copy link
Member Author

Yep, in hindsight that should have been nodeid, not item. 😞 See my PR.

@RonnyPfannschmidt
Copy link
Member

@nicoddemus since we don't have a process for experimenting with api and pluggy doesn't have a tool for renames or changes of hook parameters, a new hook name is needed

@nicoddemus
Copy link
Member Author

Perhaps, but as it stands at least the warnings are displayed, so I think the current PR is an improvement over not showing anything.

@RonnyPfannschmidt
Copy link
Member

None instead of a item is a breach of api

@nicoddemus
Copy link
Member Author

Not according to the docs:

item (pytest.Item|None) – The item being executed if when is "runtest", otherwise None.

So None is definitely a possible value for item, and hook implementations should deal with it accordingly.

@nicoddemus
Copy link
Member Author

Just to be clear: I'm not saying this is perfect, but we have to take in account a few points:

  1. This hook was just introduced, so I think only pytest itself uses it at the moment.
  2. The long term solution is to either introduce a new hook as you suggest or a new nodeid parameter to the existing hook, possibly deprecating item (we will need to introduce such support to pluggy).

All of the above are very long term (at least until the next minor release cycle), so the approach proposed here is not a breach of API (according to the documentation) and at least will show warnings from workers. The price? Well, item will always be None when using xdist, which pytest already handles gracefully and as said I suspect no other plugins make use of this hook anyway.

Because of those reasons I still think the current PR is an improvement over the current state of things.

@RonnyPfannschmidt
Copy link
Member

Indeed my mistake, if none is valid its good to go in

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

No branches or pull requests

2 participants