test(app-root): close two blind spots in the subdirectory redirect tests#42016
Open
sadpandajoe wants to merge 2 commits into
Open
test(app-root): close two blind spots in the subdirectory redirect tests#42016sadpandajoe wants to merge 2 commits into
sadpandajoe wants to merge 2 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #42016 +/- ##
==========================================
- Coverage 65.05% 64.49% -0.57%
==========================================
Files 2747 2747
Lines 153842 153843 +1
Branches 35270 35268 -2
==========================================
- Hits 100088 99221 -867
- Misses 51844 52712 +868
Partials 1910 1910
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Both tests passed while failing to check what they were named for. The sanctioned-callers scan for `get_explore_redirect_url` skipped `superset/views/utils.py` wholesale to avoid matching the helper's own definition. That also hid any genuine new *call* added inside that module — precisely the regression the scan exists to catch. Exclude the definition by shape (a lookbehind on `def `) rather than by path, so the file is scanned like any other. Verified by injecting a caller into `views/utils.py`: the scan now fails and names it. The unsafe-redirect test asserted only `status_code != 302`. The external-warning branch calls `render_app_template`, which needs a Jinja loader the minimal test app does not have, so the branch actually raised `TemplateNotFound` and returned 500 — and 500 satisfies `!= 302`. The test would equally have passed on a 404 or an unrelated crash, so it never distinguished "the user was shown a warning" from "the handler blew up". Stub `render_app_template`, then assert the status, the rendered body and the absence of a `Location` header. Extend it over the protocol-relative and external-host shapes, which gives the warning-page branch its first real coverage here. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
sadpandajoe
force-pushed
the
test/harden-subdirectory-redirect-tests
branch
from
dc10400 to
f982d02
Compare
The sanctioned-callers scan matched `get_explore_redirect_url(` as raw text, so a mention of the helper in a comment, docstring, or string literal counted as a call site. Since this test scans `views/utils.py` (the definition module), an innocent comment there would have added the file to `callers` and failed CI for no reason. Walk the AST and count only `ast.Call` nodes. This also drops the `(?<!def )` lookbehind: the definition parses to a `FunctionDef`, not a `Call`, so it is excluded by shape rather than by text, while the module stays in scope — a genuine new call added inside it is still caught. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
Bito Review Failed - Technical Failure Bito encountered technical difficulties while starting a code review session. To retry, type |
Contributor
Code Review Agent Run #351665Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
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.
SUMMARY
Two tests in the subdirectory-deployment suite passed for the wrong reason. Neither
would have caught the regression it was written to guard against. Test-only — no
production code changes.
1. The sanctioned-callers invariant scan skipped an entire module.
test_get_explore_redirect_url_sanctioned_callersscans the source tree to assertthat
get_explore_redirect_urlis called from exactly two places. To avoid matchingthe helper's own
defline, it skippedsuperset/views/utils.py— the definingmodule — wholesale. That also blinded it to any genuine call added inside that same
module, which is precisely what the scan exists to catch.
It now walks the AST and counts only
ast.Callnodes, so the defining module isscanned like any other. The definition is a
FunctionDef, not aCall, so it isexcluded by shape rather than by path — no path special-casing is needed at all.
Parsing rather than pattern-matching also means a mention of the helper in a comment,
docstring, or string literal cannot be mistaken for a call site, which matters
precisely because bringing
views/utils.pyinto scope is what this change does.Mutation-verified in both directions: injecting a call to
get_explore_redirect_urlinto a module now fails the assertion, where the old scan passed silently; and a file
that names the helper only in a comment, docstring, or string literal is correctly
not counted as a caller.
2. The unsafe-redirect test could not distinguish a safe rejection from a crash.
test_protocol_relative_url_falls_through_to_warning_page_under_subdirasserted onlystatus_code != 302. The external-URL branch callsrender_app_template, which needsa Jinja loader the minimal test app does not have — so the request was actually raising
TemplateNotFoundand returning500.500 != 302, so the test passed, whileasserting nothing about the behavior it names.
render_app_templateis now stubbed, and the test asserts the status code, the responsebody, and the absence of a
Locationheader. It's parametrized over three unsafeshapes: protocol-relative (
//host), backslash-prefixed (\\host), and an absolute URLon a non-configured host. This gives the warning-page branch its first real coverage in
this module.
TESTING INSTRUCTIONS
pytest tests/unit_tests/views/test_redirect_view_subdirectory.py— 12 pass (was 10;the one weak test became three real ones).
pytest tests/integration_tests/views/test_explore_redirect.py— 16 pass.To confirm the hardened scan actually bites, temporarily add a call to
get_explore_redirect_urlinsidesuperset/views/utils.pyand re-run — it fails withviews/utils.pylisted among the callers. To confirm it does not over-match, add afile that merely mentions
get_explore_redirect_url()in a comment or string — thescan ignores it.
ADDITIONAL INFORMATION