Conversation
Distilled from #1245 by Andrew Hundt (a slice of d15071c), with a different rule than upstream chose. Python's unittest.mock is used as a decorator: @patch("subprocess.run") def test_runs(self, run): ... decorator_method_name maps a bare `patch` to the HTTP method PATCH, and try_route_from_decorator_call accepts a receiver-less decorator call. When extract_route_path_from_args then finds nothing path-shaped, the function falls back to "/". So every mock-decorated test function became a Route handler, PATCH "/", and every one of those was wrong. Upstream fixed it with a has_receiver heuristic. That still admits @mock.patch("os.getcwd") (it has a receiver) and would reject the bare framework decorators Litestar and BlackSheep use, @get("/x"). The rule here is about the arguments, not the callee: a decorator call that HAS arguments but none of them path-shaped is not a route. The "/" default survives only for a zero-argument call such as @app.route(), which is the one case where "/" is what the framework means. No test on main relied on the default for a call with non-path arguments; checked before changing it. RED before the fix: extract_python_mock_patch_is_not_route FAIL tests/test_extraction.c:4023: mocked->route_path is not NULL extract_python_bare_decorator_route_rules FAIL tests/test_extraction.c:4063: mocked->route_path is not NULL The second test also pins the two shapes that must keep working: bare @get("/health") still yields a GET route, and zero-argument @app.route() still yields "/" ANY -- those assertions passed before the fix and after. GREEN after: extraction 351 passed; extraction pipeline edge_types_probe route_canon cross_repo infrascan lang_contract 754 passed, with the CALLS-breadth contract at 54 languages / 0 failures. The single remaining red in both runs is extract_spill_round_trip_keeps_every_field refusing to spill onto a disk below its 10 GB floor -- this host, not this change. Co-authored-by: Andrew Hundt <ATHundt@gmail.com> Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Distilled from #1245 by @ahundt (a slice of d15071c), carried with
Co-authored-by— with a different rule than upstream chose. #1245 stays open until its distills land.The bug
decorator_method_namemaps a barepatchto the HTTP verb PATCH;try_route_from_decorator_callaccepts receiver-less decorators; and when no path-shaped argument is found, it falls back to"/". So everyunittest.mock-decorated test function became a Route handler,PATCH "/".The rule, and why it differs from upstream
Upstream used a
has_receiverheuristic. That still admits@mock.patch("os.getcwd")(it has a receiver) and would reject the bare framework decorators Litestar and BlackSheep use (@get("/x")).This rule is about the arguments, not the callee: a decorator call that has arguments but none path-shaped is not a route. The
"/"default survives only for a zero-argument call like@app.route(), where"/"is what the framework means. I checked that no test on main relied on the default for a call with non-path arguments before changing it. The production change is three lines.RED → GREEN
Before the fix:
The second test also pins the shapes that must keep working: bare
@get("/health")still yields a GET route, zero-argument@app.route()still yields"/"ANY — those assertions passed before and after.After:
extraction351 passed;extraction pipeline edge_types_probe route_canon cross_repo infrascan lang_contract754 passed, CALLS-breadth contract 54 languages / 0 failures.The single remaining red in both runs is
extract_spill_round_trip_keeps_every_fieldrefusing to spill onto a disk below its 10 GB floor — this host, not this change; it fails identically before and after.One edge not tested:
@app.route( # comment\n)— tree-sitter may count the comment as a named child of the argument list, which would make it a non-route instead of"/". Judged out of scope; flagging so it is not a surprise.