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

Extend test suite and remove deprecated code #133

Merged
merged 36 commits into from
Feb 25, 2021

Conversation

northernSage
Copy link
Member

@northernSage northernSage commented Feb 10, 2021

This PR aims to extend our current test suite, getting coverage up to 100%. Also, live_server.url method has been deprecated for quite a while now, I'll probably remove it and roll the changes in this PR as well. Having better coverage will help sorting out changes proposed in #47 which touch some core functionality.

  • 100% coverage
  • remove deprecated code
  • add changelog entry
  • move helpers to new _internal.py module
  • account for multiprocessing in coverage
  • deprecate request_ctx fixture
  • remove JSONResponse.json since the functionality is now natively provided
  • update documentation

@northernSage
Copy link
Member Author

northernSage commented Feb 10, 2021

Just realized the low coverage was due to pytest-cov reporting import statements as not covered. I have updated tox.ini to address this and adjustedcontributing.rst coverage instructions to reflect the changes :bowtie:

Our actual coverage looks much better

pytest_flask/__init__.py 100%
pytest_flask/_internal.py 100%
pytest_flask/_version.py 100%
pytest_flask/fixtures.py 94%
pytest_flask/live_server.py 93%
pytest_flask/plugin.py 91%
pytest_flask/pytest_compat.py 75%
Total 93%

Regardless, deprecated code still needs to be removed and there are some small changes I would like to propose concerning how our code is currently organized, so I'll keep working on the PR.

@northernSage
Copy link
Member Author

northernSage commented Feb 18, 2021

pytest-cov was reporting all import statements as not covered. Since no particular feature offered by pytest-cov is being used (unless I'm missing something), the simplest solution was cutting off the extra dependency and using coverage commands directly. Also, since LiveServer runs on a separate process, some of its code was being reported as not covered. This is now treated with coverage --concurrency=multiprocessing.

@nicoddemus
Copy link
Member

the simplest solution was cutting off the extra dependency and using coverage commands directly

Agreed, when testing plugins and pytest itself, often is better to use coverage directly, as pytest-cov (being a plugin) ends up importing parts of pytest that we want to measure coverage for, before enabling coverage, causing those to be missed.

@northernSage
Copy link
Member Author

northernSage commented Feb 19, 2021

From werkzeug changelogs

The test Client request methods (client.get, etc.) always return an instance of TestResponse. In addition to the normal behavior of Response, this class provides request with the request that produced the response, and history to track intermediate responses when follow_redirects is used

Werkzeug 2.0.0 fails our pre build Following is the test that was failing and the error

def test_request_ctx_is_kept_around(self, client):
    client.get(url_for("index"), headers=[("X-Something", "42")])
    assert request.headers["X-Something"] == "42"
def __getitem__(self, key, _get_mode=False):
    # _get_mode is a no-op for this class as there is no index but
    # used because get() calls it.
    if not isinstance(key, str):
        raise KeyError(key)
    key = key.upper().replace("-", "_")
    if key in ("CONTENT_TYPE", "CONTENT_LENGTH"):
        return _unicodify_header_value(self.environ[key])
>       return _unicodify_header_value(self.environ[f"HTTP_{key}"])
E       KeyError: 'HTTP_X_SOMETHING'

.tox/pre/lib/python3.9/site-packages/werkzeug/datastructures.py:1369: KeyError

It seems the request context is not persisting and because of that the headers are not being found in the WSGI environ. For now I have just documented the code since I'm not sure how big of a change handling this will entail and it's probably out of this PR's scope.

def test_request_ctx_is_kept_around(self, client):
    res = client.get(url_for("index"), headers=[("X-Something", "42")])
    """In werkzeug 2.0.0 the test Client provides a new attribute 'request'
    in the response class which holds a reference to the request object that
    produced the respective response, making introspection easier"""
    try:
        assert res.request.headers["X-Something"] == "42"
    except AttributeError:
        """This is the conventional (pre 2.0.0) way of reaching the
        request object, using flask.request global."""
        assert request.headers["X-Something"] == "42"

Suggestions are welcome in case anyone feels like jumping in

@northernSage
Copy link
Member Author

Also, the request_ctx fixture feels unnecessary now since you can just get the requeste context from response.request, so I added a deprecation warning.

@northernSage
Copy link
Member Author

It appears Werkzeug's response class now has its own json property that does exactly what our JSONResponse.json does. The __eq__ and __ne__ patches are still required, though.

That's how our coverage looks like at the moment

pytest_flask/init.py | 1 | 0 | 0 | 100%
pytest_flask/_internal.py | 20 | 0 | 0 | 100%
pytest_flask/_version.py | 2 | 0 | 0 | 100%
pytest_flask/fixtures.py | 52 | 0 | 0 | 100%
pytest_flask/live_server.py | 55 | 0 | 0 | 100%
pytest_flask/plugin.py | 66 | 1 | 0 | 98%
pytest_flask/pytest_compat.py | 4 | 1 | 0 | 75%
Total | 200 | 2 | 0 | 99%

Almost there 👍🏻

@northernSage northernSage marked this pull request as ready for review February 23, 2021 08:43
Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not review everything in detail, but seems good overall. Great work!

pytest_flask/plugin.py Show resolved Hide resolved
@northernSage northernSage merged commit e9b8321 into pytest-dev:master Feb 25, 2021
@northernSage northernSage deleted the test-suite-improvement branch February 25, 2021 05:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants