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

Use latest pluggy #2730

Closed
goodboy opened this issue Aug 29, 2017 · 9 comments
Closed

Use latest pluggy #2730

goodboy opened this issue Aug 29, 2017 · 9 comments
Labels
type: enhancement new feature or API change, should be merged into features branch type: feature-branch new feature or API change, should be merged into features branch type: performance performance or memory problem/improvement

Comments

@goodboy
Copy link

goodboy commented Aug 29, 2017

Latest version is now 0.5.1.

The list of benefits includes:

  • speedier _MultiCall loop using for instead of while
  • warnings for the deprecation of __multicall__
  • warnings for hook impls which mismatch specs
  • more comprehensible error messages on incorrect usage
  • important fix for callbacks where historic hooks would not be called for already registered plugins
@goodboy goodboy added type: deprecation feature that will be removed in the future type: enhancement new feature or API change, should be merged into features branch type: performance performance or memory problem/improvement labels Aug 29, 2017
@nicoddemus nicoddemus added the type: feature-branch new feature or API change, should be merged into features branch label Aug 29, 2017
@nicoddemus
Copy link
Member

Took a quick stab at this but it seems there's a problem (or misinterpretation on my part) on how firstresult works:

Traceback (most recent call last):
  File "C:\pytest\.env27\Scripts\pytest-script.py", line 11, in <module>
    load_entry_point('pytest', 'console_scripts', 'pytest')()
  File "c:\pytest\_pytest\config.py", line 50, in main
    config = _prepareconfig(args, plugins)
  File "c:\pytest\_pytest\config.py", line 160, in _prepareconfig
    pluginmanager=pluginmanager, args=args)
  File "c:\pytest\.env27\lib\site-packages\pluggy\__init__.py", line 680, in __call__
    return self._hookexec(self, self._nonwrappers + self._wrappers, kwargs)
  File "c:\pytest\.env27\lib\site-packages\pluggy\__init__.py", line 240, in _hookexec
    return self._inner_hookexec(hook, methods, kwargs)
  File "c:\pytest\.env27\lib\site-packages\pluggy\__init__.py", line 234, in <lambda>
    methods, kwargs, specopts=hook.spec_opts, hook=hook
  File "c:\pytest\.env27\lib\site-packages\pluggy\callers.py", line 104, in execute
    gen.send(outcome)
  File "c:\pytest\_pytest\helpconfig.py", line 70, in pytest_cmdline_parse
    if config.option.debug:
AttributeError: 'list' object has no attribute 'option'

Here's the code for pytest_cmdline_parse() impl:

@pytest.hookimpl(hookwrapper=True)
def pytest_cmdline_parse():
outcome = yield
config = outcome.get_result()
if config.option.debug:
path = os.path.abspath("pytestdebug.log")

outcome.get_result() is returning a list, while we expected it to return a single Config object.

That's using pluggy-0.5.1.

@RonnyPfannschmidt
Copy link
Member

@tgoodlet yay - this means we missed and important core detail of pluggy that @hpk42 never had a correct test for ^^ and 0.5.1 is not actually good, just not completely broken

@goodboy
Copy link
Author

goodboy commented Aug 30, 2017

@nicoddemus hah! Nice catch.
This firstresult stuff seems to be quite subtle (and untested) hehe.

I think the difference here is that the way the original _LegacyMultiCall terminates its while loop when firstresult is used. The recursion unwinds returning a single value in a very sneaky way.

I can see 2 ways to handle this:

  • make _Result aware of the firstresult notion
  • handle and reset the outcome based on the firstresult condition before sending to hookwrappers

I personally vote for the latter.

I'll suggest a test and fix first tomorrow later this morning.
It's a good thing we're catching this stuff now so we can add tests for it 👍

@goodboy
Copy link
Author

goodboy commented Aug 30, 2017

@RonnyPfannschmidt yes you're totally right.
I guess it's not all bad because at least we're figuring out the tests we need.
This is why I was partial a while back to running pytest unit tests as part of pluggy CI.

Either way sounds like we'll have 2 patch releases in 2 days...yey

@RonnyPfannschmidt
Copy link
Member

@tgoodlet well its not too bad because there is no pytest release yet that we did break, but its a pain that we cause tox/devpi users trouble

@goodboy
Copy link
Author

goodboy commented Aug 30, 2017

@RonnyPfannschmidt yeah definitely not great.
At least it seems like none of those projects are using firstresult hook wrappers.

Alright looking at a fix.

@goodboy
Copy link
Author

goodboy commented Aug 30, 2017

@RonnyPfannschmidt @nicoddemus please see pytest-dev/pluggy#72

Sorry about the lack of thoroughness with this. I should really at least run the pytest suite manually before making a pluggy release.

Would you guys be ok if I did run the latest version of pytest as part of the CI job from now on?

@RonnyPfannschmidt
Copy link
Member

@tgoodlet we also need a job on pytest to test regulary with latest pluggy and in future attrs as well

@goodboy
Copy link
Author

goodboy commented Aug 30, 2017

@RonnyPfannschmidt sounds like a good plan.

@nicoddemus nicoddemus added type: enhancement new feature or API change, should be merged into features branch and removed type: enhancement new feature or API change, should be merged into features branch labels Sep 14, 2017
@nicoddemus nicoddemus removed the type: deprecation feature that will be removed in the future label Jan 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement new feature or API change, should be merged into features branch type: feature-branch new feature or API change, should be merged into features branch type: performance performance or memory problem/improvement
Projects
None yet
Development

No branches or pull requests

3 participants