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

test: add tests in test_routing #2676

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

Orenoid
Copy link
Contributor

@Orenoid Orenoid commented Aug 31, 2024

Summary

Add tests for some of the uncovered branches in starlette.routing, related to this issue

Checklist

  • I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
  • I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • I've updated the documentation accordingly. (It seems no documentation needs update)

Comment on lines +527 to +528
with pytest.raises(NoMatchFound):
mixed_hosts_app.url_path_for("api", path="whatever", foo="bar")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Testing the circumstance where there are some remaining parameters and an exception is expected.
https://github.com/encode/starlette/blob/master/starlette/routing.py#L532

Copy link
Contributor Author

@Orenoid Orenoid Sep 1, 2024

Choose a reason for hiding this comment

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

I passed an extra parameter foo in this case. This will make the if statement become False and jump to this line:

raise NoMatchFound(name, path_params)

This behavior was not tested and covered in the other tests before.

Comment on lines +420 to +426
mounted = Router([Mount("/users", ok, name="users")])
with pytest.raises(NoMatchFound):
mounted.url_path_for("users", path="/a", foo="bar")

mounted = Router([Mount("/users", ok, name="users")])
with pytest.raises(NoMatchFound):
mounted.url_path_for("users")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Orenoid Orenoid changed the title test: add tests in test_routing test: add tests in test_routing Aug 31, 2024
@Kludex
Copy link
Member

Kludex commented Sep 1, 2024

Can you explain me what behavior is being tested?

@Orenoid
Copy link
Contributor Author

Orenoid commented Sep 1, 2024

Can you explain me what behavior is being tested?

I've provided the code lines above. According to the coverage report, those if statements are always True with the other tests. The tests I added check if the logic can correctly jump to another branch, which should raise an exception in these cases.

@Orenoid Orenoid mentioned this pull request Sep 7, 2024
3 tasks
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.

2 participants