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

Add test coverage for HeaderElement and AcceptElement #2027

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

radez
Copy link
Contributor

@radez radez commented Apr 2, 2024

This is in preparation for issue #2014
Establishing tests to validate these objects so we can pull in the deprecated cgi.parse_header code

What kind of change does this PR introduce?

  • tests/coverage improvement

This is in preparation for issue cherrypy#2014
Establishing tests to validate these objects so we can
pull in the deprecated cgi.parse_header code
@radez
Copy link
Contributor Author

radez commented May 21, 2024

@webknjaz I pushed up some updates to address comments last week. Do I need to do anything else for this to go in?

@webknjaz
Copy link
Member

Thanks for reminding! I'm going to take another look at this now as I'm on the PyCon US Sprints.

Copy link
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

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

So.. I identified a few problems here. I'll try applying some patches and rebasing myself and will see where to go from there. Stay tuned!

cherrypy/test/test_httputil.py Outdated Show resolved Hide resolved
def test_accept_element_raises_400(header_content, media_type, qvalue):
""" Test that headers passed to AcceptElement from httputils
raise an expected HTTPError """
httputil.AcceptElement.from_str(header_content)
Copy link
Member

Choose a reason for hiding this comment

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

Given the other comment I made, I'm surprised that this doesn't fail in the CI — it seems like you expect that it would.

cherrypy/test/test_httputil.py Outdated Show resolved Hide resolved
cherrypy/test/test_httputil.py Outdated Show resolved Hide resolved
cherrypy/test/test_httputil.py Outdated Show resolved Hide resolved
cherrypy/test/test_httputil.py Outdated Show resolved Hide resolved
cherrypy/test/test_httputil.py Outdated Show resolved Hide resolved
cherrypy/test/test_httputil.py Outdated Show resolved Hide resolved

The expected exception is :exc:`~cherrypy.HTTPError`.
"""
with pytest.raises(
Copy link
Member

Choose a reason for hiding this comment

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

@radez so as I implemented this, the problem with the expectation in the test got evident. Could you see if you can identify what inputs would actually lead to the validation error expected here...

@webknjaz webknjaz changed the title Adding test coverage for HeaderElement and AcceptElement Add test coverage for HeaderElement and AcceptElement May 21, 2024
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.

None yet

2 participants