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

Explicitly store request data #78

Merged
merged 3 commits into from
Mar 20, 2024
Merged

Conversation

a-gerhard
Copy link
Contributor

Closes #77

Copy link
Contributor

@diazona diazona left a comment

Choose a reason for hiding this comment

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

Thanks! Mostly looks good but I left a few suggestions. I think it would also be best if the commit message could be expanded a bit to describe what is being done here at a high level.

.gitignore Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
tests/test_http.py Outdated Show resolved Hide resolved
tests/test_http.py Outdated Show resolved Hide resolved
pytest_localserver/http.py Outdated Show resolved Hide resolved
@diazona diazona added this to the v1.0 milestone Mar 19, 2024
@a-gerhard
Copy link
Contributor Author

I added the changes you requested.

@a-gerhard
Copy link
Contributor Author

I didn't check every test set, but it looks like the failures are the only ones using werkzeug v1, which doesn't have the json property on Request objects..

I'd say that this would be an issue of the test using httpserver rather than an issue of httpserver itself.

  • Should there even be a test for the json property, as we can easily assume that it's covered by werkzeug?
  • If we do keep the json test, should the test make a check for the property, and in this case test for the data instead?

@diazona
Copy link
Contributor

diazona commented Mar 19, 2024

I think it's okay to drop the JSON test, because as you mentioned, it should be pretty safe to assume Werkzeug ensures that if data works then json will also work. (And I think there are also other properties or methods that read the body by using get_data() under the hood, aren't there? text and such... no particular reason I can think of to single out json as the one "higher level" property to test.)

Another option would be to make the test conditional on the Werkzeug version, probably using importlib_metadata to check the version and pytest.skipif to implement the conditional check. Or just catch the AttributeError and call pytest.skip() from inside the test. It may or may not be worth the effort, but if you want to implement something like this, I'm happy to have it and keep the JSON test. I don't think this project is at a point where we need to be picky about which test cases to include.

@a-gerhard
Copy link
Contributor Author

I decided to drop test, as it would be pretty complicated for something that doesn't really add any benefit.

Copy link
Contributor

@diazona diazona left a comment

Choose a reason for hiding this comment

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

Thanks! One last thing I only just noticed, could you add yourself to the AUTHORS file? (as a separate commit should be fine) Then this will be good to go.

Other than that, the one remaining nice-to-have item would be to expand the commit message for the commit that adds store_request_data, but given that we don't have any documented guidance on commit messages I'm not considering that a blocker.

@a-gerhard
Copy link
Contributor Author

Should I fix that line that's too long for flake8? It's not part of my changes.

@diazona
Copy link
Contributor

diazona commented Mar 20, 2024

Nah don't worry about it. I'll take care of fixing the remaining pre-commit failures in another PR. For now I can force-merge this one for you. (If necessary - I guess the pre-commit failure might not even be a blocker yet)

@diazona diazona merged commit 6782a31 into pytest-dev:master Mar 20, 2024
51 of 52 checks passed
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 this pull request may close these issues.

httpserver.requests[-1].json raises ClientDisconnected
2 participants