Add regression test for #5621 #5635
Conversation
|
master version of #5633 |
|
@thehesiod could you drop those unrelated file changes with typings? |
|
Also, the linter is unhappy :( Those annoying style issues should be fixed... |
psf-chronographer
bot
added
the
bot:chronographer:provided
if I don't keep those there are PEP typing warnings in the file |
|
sigh, not sure why mypy is complaining, pycharm is happy, seems like it doesn't recognize Union, I'll revert then and keep the warning |
|
ah, figured it out |
aiohttp/test_utils.py
Outdated
| @@ -144,14 +144,14 @@ async def start_server(self, **kwargs: Any) -> None: | |||
| async def _make_runner(self, **kwargs: Any) -> BaseRunner: | |||
| pass | |||
|
|
|||
| def make_url(self, path: str) -> URL: | |||
| def make_url(self, path: Union[str, URL]) -> URL: | |||
There was a problem hiding this comment.
If these changes are necessary, please submit them in a separate PR with a separate changelog fragment.
Codecov Report
@@ Coverage Diff @@
## master #5635 +/- ##
=======================================
Coverage 97.17% 97.17%
=======================================
Files 41 41
Lines 8865 8865
Branches 1425 1425
=======================================
Hits 8615 8615
Misses 133 133
Partials 117 117
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
tests/test_web_urldispatcher.py
Outdated
| async def handler(_): | ||
| return web.Response() | ||
|
|
||
| app.router.add_get("/467%2C802%2C24834%2C24952%2C25362%2C40574/hello", handler) |
There was a problem hiding this comment.
Why doesn't this one have a regex? If the reproducer doesn't depend on it, why don't we just remove it from the first test too? Also, the URL is quite long and since the test doesn't seem to be influenced by its length, let's just make it 2-character, one char would be ascii and the other one would be something urlencoded — this will reduce the cognitive load for the readers.
There was a problem hiding this comment.
it doesn't need a regex, this is ensuring you can't have a match to an escaped path
There was a problem hiding this comment.
I have a feeling that the first test doesn't need that either. Just register a decoded route, right?
There was a problem hiding this comment.
correct, I've written it to match the actual bug in our production system. I think it's worth keeping both ways to ensure both the regex works and the non regex, resulting in more code coverage
There was a problem hiding this comment.
ok cool, I wasn't sure if you guys like separate tests or combo tests
There was a problem hiding this comment.
Okay, then, please incorporate the earlier parametrization suggestions and keep both cases as params.
Co-authored-by: Sviatoslav Sydorenko <[email protected]>
Co-authored-by: Sviatoslav Sydorenko <[email protected]>
Co-authored-by: Sviatoslav Sydorenko <[email protected]>
Co-authored-by: Sviatoslav Sydorenko <[email protected]>
Backport to 3.8: 💚 backport PR created✅ Backport PR branch: Backported as #5636 🤖 @patchback |
Co-authored-by: Sviatoslav Sydorenko <[email protected]> (cherry picked from commit 09ac1cb)
Co-authored-by: Sviatoslav Sydorenko <[email protected]> Co-authored-by: Alexander Mohr <[email protected]>
Co-authored-by: Sviatoslav Sydorenko <[email protected]>
Co-authored-by: Sviatoslav Sydorenko <[email protected]>
What do these changes do?
Adds regression test for issue #5621
Are there changes in behavior for the user?
Related issue number
#5621
Checklist
CONTRIBUTORS.txtCHANGESfolder<issue_id>.<type>for example (588.bugfix)issue_idchange it to the pr id after creating the pr.feature: Signifying a new feature..bugfix: Signifying a bug fix..doc: Signifying a documentation improvement..removal: Signifying a deprecation or removal of public API..misc: A ticket has been closed, but it is not of interest to users.