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

Declare PROVISIONAL status for test_utils and pytest plugins #1154

Closed
asvetlov opened this issue Sep 11, 2016 · 8 comments
Closed

Declare PROVISIONAL status for test_utils and pytest plugins #1154

asvetlov opened this issue Sep 11, 2016 · 8 comments
Labels

Comments

@asvetlov
Copy link
Member

asvetlov commented Sep 11, 2016

I've made a mistake by accepting #902 and #914 without very careful review and adopting aiohttp tests to using this technologies.

Both was published in 0.22 release.

But after this on test rewriting others and I have found some quirks.

We have fixed one of them: after #1083 test_client fixture accepts app as well as factory.
I believe the factory should be deprecated: the new api is much simpler and obvious.

After that I've split TestClient functionality into itself and TestServer -- all is done for good and doesn't bring backward compatibility.

The next spike is: on testing client I want to pass custom connector instance into the test_client fixture.
We could add a connector param only if the first parameter is web.Application instance, not factory.

But it's brings cognitive mess again. Maybe we'll find another problems in testing API in the near future.

Conclusion
Sorry for so long introduction.

I propose to:

  • Declare testing API as provisional.
  • Feel free to break the API in next aiohttp release (but keep trying to support it at least for next release).
  • Our current deprecation period is about 1.5 years for stable code. For testing API it could be much shorter on real use case demand (half an year probably).

Objections?

P.S.
@toumorokoshi and @samuelcolvin please don't get me wrong.
I've been very excited by your contributions into aiohttp testing tools.
The requested feature had had existed for very long ago.
I very appreciate your help.
Don't know when I was able to invest a time into the problem.
I just want to thank you.

But the first pancake is always lumpy, you know.

I have a feeling the support period for Test API solutions which are proven as not perfect should be reduced.
And we should not hesitate to break test API if keeping support of already presented solutions is not possible or maintenance burden is too high.

@toumorokoshi
Copy link
Contributor

👍 on declaring the api provisional. I think aiohttp's test utilities are already pretty good, but I agree that we shouldn't lock ourselves into test patterns that we've only tried out for a little while. As long as the apis are available, I'm happy (already using them :) )

I propose we just label all test apis provisional for now, until there's agreement among early adopters that this is the right way to go.

I also agree that the connector parameter you propose is a little concerning: I personally hate function behavior that changes based on the type of object passed: it makes it harder to reason about what the function is actually doing. would it be possible to just provide two different constructors instead (static methods from_app and from_connector, or something like that).

@samuelcolvin
Copy link
Member

👍 fine with my.

I would strongly suggest keeping the direct integration with pytest, it's by far the most user friendly and powerful test framework for python and having direct integration means aiohttp can do a much better job of holding people's hands as they start out with unit tests. But keeping the API provisional is fine.

Splitting TestClient and TestServer makes sense.

@asvetlov
Copy link
Member Author

asvetlov commented Sep 12, 2016

@samuelcolvin I love to support pytest.
You know there is https://github.com/aio-libs/pytest-aiohttp

The library is just very trivial stub for getting rid of pytest_plugins = 'aiohttp.pytest_plugin'.

The actual code is:

__version__ = '0.1.3'
from aiohttp.pytest_plugin import *

I'm working hard on converting aiohttp tests into pytest way.
It's very boring and time consuming job and any help is very appreciated.

@samuelcolvin
Copy link
Member

Why another library outside aiohttp?

Also isn't pytest_plugins ... the canonical way of enabling a plugin?

@asvetlov
Copy link
Member Author

asvetlov commented Sep 12, 2016

pip install pytest-* is an alternative way for plugin enabling :)
It doesn't require pytest_plugins ... in conftest.py but just works.

Unfortunately due weird setuptools circular dependencies aiohttp cannot declare pytest plugin for itself and use it in self tests.

I recall a mention of the problem in pytest docs but cannot find it now unfortunately.

Maybe I'm wrong but just adding

    entry_points={
        'pytest11': ['aiohttp = pytest_aiohttp'],
    },

into aiohttp's setup.py didn't work.

Also it's rude to add pytest as install requirement for aiohttp -- not every user need it and moreover test dependency is definitely odd for production environments.

@samuelcolvin
Copy link
Member

makes sense.

@asvetlov
Copy link
Member Author

Fixed by #1159

@lock
Copy link

lock bot commented Oct 29, 2019

This thread has been automatically locked since there has not been
any recent activity after it was closed. Please open a new issue for
related bugs.

If you feel like there's important points made in this discussion,
please include those exceprts into that new issue.

@lock lock bot added the outdated label Oct 29, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

3 participants