[fix] Raise AppTestError for unsupported AppTest interactions - #16837
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
✅ PR preview is ready!
|
Added Agent Docs
|
|
| Filename | Overview |
|---|---|
| lib/streamlit/testing/v1/element_tree.py | Adds explicit handling and contextual error guidance for unsupported AppTest interactions. |
| lib/streamlit/testing/v1/errors.py | Updates the AppTestError documentation to include unsupported modeled interactions. |
| lib/tests/streamlit/testing/element_tree_test.py | Verifies unsupported interaction errors, guidance text, typed-method behavior, and protobuf field access. |
Reviews (3): Last reviewed commit: "Clarify inspectable-only AppTestError wo..." | Re-trigger Greptile
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
Reviewed by Cursor Bugbot for commit 0da0fcb. Configure here.
There was a problem hiding this comment.
Summary
AppTest now raises AppTestError when a tester calls set_value() or click() on an element with no typed interaction method. Previously, Element.__getattr__ leaked pagination’s protobuf set_value: bool field, so at.get("pagination")[0].set_value(2) failed with TypeError: 'bool' object is not callable; display elements such as markdown failed with AttributeError. Both paths now fail with the same domain-specific exception used for other invalid AppTest actions (for example, updating a disabled widget).
The diff is small (~25 lines of production code) and proportionate: one guard in the existing fallback path, no new abstractions or state. All three reviews agree the core fix is correct and well-scoped. They disagreed on whether an inaccurate click() diagnostic for typed widgets that already support set_value() should block merge; that is a wording gap on an already-invalid call, not a functional regression (see inline).
Product Alignment
This is user-facing in the narrow sense that it changes the exception type and message on the public streamlit.testing.v1 AppTest surface. It is a bug fix, not a new command, widget, or significant API, so a product spec is not required.
The merged pagination spec at specs/2026-03-22-st-pagination defines browser pagination behavior, not AppTest interaction support. The PR body does not claim extra product approval; the linked P0.2 wiki queue is tracking context, not a product decision.
The change matches existing AppTest patterns and the “Fail Fast, Fail Helpfully” principle: invalid tester actions raise AppTestError with element type, optional key, and a pointer to a typed wrapper or Playwright. Reusing AppTestError rather than a new exception class matches Widget._assert_can_interact and keeps the public surface small.
A typed Pagination AppTest wrapper (keyed widget with a session-state value) would make this error path unnecessary for the element that motivated the fix. That is a reasonable follow-up, not a requirement for this stopgap.
Code Quality
The intercept lives in Element.__getattr__, so Widget.set_value and Button / DownloadButton / MenuButton.click still win via normal attribute lookup. Returning a callable (instead of raising inside __getattr__) is the right shape: hasattr will not blow up with a non-AttributeError, and the failure happens at the call site.
Two cross-cutting behavior notes that are acceptable but undocumented in the PR:
- After this change,
hasattr(element, "set_value")andhasattr(element, "click")returnTruefor every element, includingMarkdown. Nothing inlib/streamlit/testing/duck-types on those names today; user helpers that branched onhasattrwould move from “skip” to “raise at call time.” - Reading
element.set_valueas a proto bool onUnknownElementwidgets no longer works (a callable is returned instead). Testers who need that internal flag can still usenode.proto.set_value.set_valueis not a publicst.paginationparameter.
Defining set_value / click on Element that raise, and letting widget subclasses override them, would avoid a nested function in __getattr__. That is optional; the current helper is enough for this bug. The type/key interpolation duplicates Widget._assert_can_interact; two occurrences do not yet justify a shared helper.
Performance
No meaningful impact. Element.__getattr__ is only reached for attributes not found through normal lookup and is off every backend hot path in lib/streamlit/AGENTS.md (element creation, ForwardMsg serialization, reruns, session state). The added work is a set-membership check on a string; the closure is allocated only when the name matches. There are no frontend changes.
Test Coverage
The new unit test in lib/tests/streamlit/testing/element_tree_test.py follows lib/tests/AGENTS.md conventions and covers the primary regression (pagination’s proto bool must not leak as a callable) plus a display-element path (markdown). Asserting node.value == 1 and node.key == "pager" pins that the element stays inspectable while rejecting interaction. Existing widget tests still cover successful set_value / click.
E2E coverage is correctly out of scope: AppTest behavior is not observable in the browser.
Backwards Compatibility
Valid AppTest interactions are unchanged. Invalid calls that used to raise TypeError (pagination set_value) or AttributeError (markdown set_value, widgets without click) now raise AppTestError. Tests that asserted those older exception types would need to catch AppTestError instead. That is an acceptable testing-API correction: those calls were never supported.
Typed widget classes are unaffected — real methods resolve before __getattr__ runs.
Security & Risk
No security surface is touched. The change is confined to the AppTest harness (lib/streamlit/testing/), which does not run in production apps: no websocket, endpoint, auth, session, upload, asset-serving, cookie, CORS, sanitization, iframe, postMessage, subprocess, or dependency changes. Regression risk is limited to test code that relied on the previous exception types, discussed above.
External test recommendation
- Recommend external_test: No
- Triggered categories: None
- Evidence:
lib/streamlit/testing/v1/element_tree.py: AppTest-only__getattr__intercept for unsupportedset_value/clicklib/streamlit/testing/v1/errors.py:AppTestErrordocstring onlylib/tests/streamlit/testing/element_tree_test.py: in-process AppTest unit coverage
- Suggested external_test focus areas:
- None; this does not affect hosted, embedded, or proxied Streamlit sessions
- Confidence: High
- Assumptions and gaps: Assessment is limited to the three-file diff; no runtime, routing, or frontend files changed. Assumption: no hosted product surfaces
AppTestto end users at runtime, which holds for the Streamlit library.
Accessibility
Not applicable. No frontend or rendered UI changes.
Readability
Line-level docstring, test-name, and error-message wording notes are in inline comments.
PR title
“Inspectable-only widgets” is jargon, and the change also applies to non-widget elements such as markdown.
Proposed: [fix] Raise AppTestError for unsupported AppTest interactions
PR description
The body leads with the concrete symptom, which is the right shape. Two small accuracy nits:
- “Those names used to resolve to protobuf fields” is slightly imprecise — only
set_valueis a proto field;clickpreviously raisedAttributeError. - The “Implements wiki queue P0.2” sentence is tracking metadata that does not help a future reader understand the change.
Proposed rewrite of the opening:
Raises `AppTestError` when AppTest calls `set_value()` or `click()` on
elements with no typed interaction wrapper, such as `st.pagination` and
markdown. `set_value` used to resolve to a protobuf bool field (`TypeError:
'bool' object is not callable`) and `click` to `AttributeError`, so testers
got no pointer to a typed wrapper or Playwright e2e.
Optional: one sentence noting that the proto set_value field is no longer readable on UnknownElement widgets (use node.proto.set_value instead).
Recommendations
- Optional: specialize the unsupported-interaction message for typed widgets that lack
click(), and pin that path in the unit test (see inline). Not required for merge. - Optional follow-up: add a typed
Paginationwrapper to AppTest. It is a keyed widget with a session-state value, so it is a good candidate, and that would make this error path unnecessary for the element that motivated the fix. - Optional: apply the PR title and description wording above.
Verdict
APPROVED: A small, well-tested AppTest bugfix that turns a confusing TypeError/AttributeError into a clear AppTestError, with no compatibility, performance, or security risk. Two of three reviews approved; the remaining change request is diagnostic wording on an already-invalid click() call (also filed by Bugbot at low severity) and should not block merge.
This is a consolidated automated AI review by cursor-grok-4.6-xhigh. Please verify the feedback and use your judgment.
This review also includes 6 inline comment(s) on specific code lines.
Inspectable-only elements still point testers at Playwright; widgets that already support set_value() now get a matching diagnostic.
There was a problem hiding this comment.
Summary
This PR makes AppTest fail with an actionable AppTestError when a test calls set_value() or click() on a node that AppTest can inspect but cannot drive. The intercept lives in Element.__getattr__ so it wins over protobuf fields with the same name — the motivating case is st.pagination, whose proto set_value: bool previously leaked through and failed as TypeError: 'bool' object is not callable. Typed widgets keep their real methods; widgets that only lack click() get a message pointing at set_value() / typed helpers; inspectable-only nodes point testers elsewhere.
All four expected reviews completed (gpt-5.6-sol-high, claude-opus-5-thinking-high, gemini-3.7-flash-high, cursor-grok-4.6-xhigh) and all approved. Reviewers agreed on the diagnosis, error type, intercept location, test adequacy, and the absence of security, performance, accessibility, or external-test risk. They differed on error-copy accuracy for unmodeled-but-interactive widgets, whether to replace the __getattr__ stub with real raising methods, and how tightly to pin the new tests. Those disagreements are resolved below; none are merge-blocking.
Product Alignment
User-facing impact is narrow: exception type and message text on the public streamlit.testing.v1 (AppTest) surface, plus attribute access for the internal proto flag set_value. No public st.* command, parameter, config option, or rendered UI changes. Per specs/AGENTS.md ("Does not require a spec: Bug fixes… small non-controversial enhancements"), no product spec is needed. The pagination product spec at specs/2026-03-22-st-pagination/product-spec.md is already merged and covers browser behavior, not AppTest interaction support. The PR body does not claim extra product approval beyond implementing wiki queue P0.2.
This aligns with #23 Fail Fast, Fail Helpfully (replace cryptic TypeError / AttributeError with a named AppTestError and a next step) and #5 Consistency Over Novelty (AppTestError is already the "tester cannot perform this action" type, used by Widget._assert_can_interact for disabled widgets). #25 Graceful Evolution covers the two AppTest-only behavior changes (proto-field attribute and hasattr); both fail loudly rather than silently.
One reviewer flagged that the non-widget message claims the element "is not interactive," which is inaccurate for unmodeled widgets such as st.pagination — the element that motivated the PR. That is a wording refinement under #23, not a material API-principle violation; it does not block merge.
Code Quality
The change is small and lands in the right place. Intercepting in Element.__getattr__ (lib/streamlit/testing/v1/element_tree.py) is required: node.set_value(x) is getattr-then-call, so a proto bool would still be invoked unless getattr is hijacked, and raising inside __getattr__ would make hasattr throw AppTestError. Real Widget.set_value / Button.click still win via normal lookup. No protobuf defines a field named click; set_value only appears on widget protos, and every other set_value-bearing proto already has a typed wrapper.
Reviewers split on replacing the reserved-name set plus closure plus isinstance(self, Widget) with raising set_value / click methods on Element and Widget. The __getattr__ shape is the better fit: putting always-raising methods on Element would advertise click() / set_value() on markdown, captions, and every other node via dir(), IDEs, and type checkers. The isinstance branch is a mild layering shortcut and is not worth extra machinery here.
Block and its subclasses do not inherit from Element and have no proto fallback, so at.main.click() still raises a plain AttributeError. That inconsistency predates this PR and is out of scope.
Performance
No meaningful impact. This code runs only inside AppTest-based unit tests, never during app execution or message serialization. None of the backend hot paths in lib/streamlit/AGENTS.md are touched. The added work is a two-name set membership check on a path that previously did a getattr anyway; the stub is allocated only for the intercepted names. No frontend changes.
Test Coverage
Coverage is adequate for the fix. test_inspectable_elements_reject_unsupported_interactions covers the pagination proto-bool regression plus click and typed Markdown; test_typed_widget_without_click_raises_app_test_error pins the widget-specific "use set_value()" message. Existing tests still cover successful widget set_value / click and disabled-widget AppTestError. E2E tests are correctly not applicable (AppTest-only, no browser surface).
One reviewer wanted deeper message pins and an assertion that node.proto.set_value still resolves. That would strengthen the tests but is not required coverage; the current assertions already prove the regression is gone and the two message branches are distinct.
Backwards Compatibility
Supported interactions are unchanged. Previously invalid calls now raise AppTestError instead of TypeError (pagination set_value) or AttributeError (markdown / widgets without click). AppTestError does not inherit from AttributeError, so suites that caught the old incidental types will need updating. That is the intended signal.
Two related attribute-access changes, both confined to AppTest:
- Proto
set_valueis no longer readable as an attribute on unimplemented widgets. Documented in the PR body; usenode.proto.set_value. In practice this only affectsst.paginationtoday. Reading that internal flag from a test has no plausible use. hasattr(element, "click")/hasattr(element, "set_value")now returnTruefor everyElementthat does not already define those methods, because the stub is returned instead ofAttributeError. Duck-typed helpers that probed these names as capability checks can now enter the stub and getAppTestError. Same ashasattron a disabled button's realclick. Unlikely to matter in practice; worth a PR-body sentence, not a blocker.
Security & Risk
No production runtime surface. Changes are confined to lib/streamlit/testing/v1/ (in-process test harness): no websocket, routes, auth, uploads, cookies, CORS, HTML sanitization, iframe/postMessage, secrets, or new dependencies. Regression risk is limited to AppTest suites that relied on the old exception types or proto-field attribute.
External test recommendation
- Recommend external_test: No
- Triggered categories: None
- Evidence:
lib/streamlit/testing/v1/element_tree.py: AppTest-only__getattr__intercept and local error construction; no routing, transport, or embedding code.lib/streamlit/testing/v1/errors.py: docstring-only change to an exception class.lib/tests/streamlit/testing/element_tree_test.py: in-process AppTest unit tests.
- Suggested external_test focus areas: none. AppTest runs the script runner in-process with no browser, server, or cross-origin boundary.
- Confidence: High
- Assumptions and gaps: The assessment covers the complete three-file PR. AppTest is a local Python testing harness and is not part of deployed app request or browser transport paths.
Accessibility
Not applicable. No frontend or rendered UI changes.
Readability
lib/streamlit/testing/v1/element_tree.py
The __getattr__ docstring explains the protobuf collision; a reviewer noted the summary line still leads with proto fallback rather than the interaction rule (inline). _raise_unsupported_interaction is a clear name; a one-line docstring would make the Widget vs inspectable-only split obvious (inline). Naming of the new tests communicates purpose without reading the bodies.
lib/streamlit/testing/v1/errors.py
The expanded AppTestError docstring accurately frames the limitation as AppTest's (not a browser-user one) and uses concrete examples. No rewrite proposed.
PR title
[fix] Raise AppTestError for unsupported AppTest interactions stands alone, follows [type] Description, and is within length. No rewrite.
PR description
Leads with the change and why, distinguishes the prior TypeError vs AttributeError failures, documents the node.proto.set_value migration, links P0.2, and includes the required template sections. The remaining gap is the undocumented hasattr consequence (inline). UnknownElement in the proto-field paragraph is an internal name; a follow-up could say this only affects st.pagination in practice.
Recommendations
- Optional: reword the non-widget
AppTestErrorso it does not claim unmodeled widgets "are not interactive," and mentionat.session_statefor keyed widgets (see inline onelement_tree.py). - Optional: note the
hasattrbehavior change in the PR body next to the proto-attribute migration. - Optional: pin the key suffix / guidance sentence and
node.proto.set_valuein the new tests. - Follow-up (out of scope): a typed AppTest wrapper for
st.pagination(andst.link_buttonnow that it supportson_click) would close the gap this error makes discoverable. A one-sentence note inlib/streamlit/.agents/skills/developing-with-streamlit/references/testing.mdnext to the disabled-widgetAppTestErrornote would also help agents.
Verdict
APPROVED: A focused AppTest diagnostic fix with the right intercept, correct error-type reuse, and adequate unit coverage; remaining items are optional wording, PR-body, and test-pin nits.
This is a consolidated automated AI review by cursor-grok-4.6-xhigh. Please verify the feedback and use your judgment.
This review also includes 5 inline comment(s) on specific code lines.
There was a problem hiding this comment.
Auto-approved by AI final review (workflow run).

Describe your changes
Raises
AppTestErrorwhen AppTest callsset_value()orclick()onelements with no typed interaction wrapper, such as
st.paginationandmarkdown.
set_valueused to resolve to a protobuf bool field (TypeError: 'bool' object is not callable) andclicktoAttributeError, so testersgot no pointer to a typed wrapper or Playwright e2e.
The protobuf
set_valuefield onUnknownElementwidgets is no longer readable as an attribute; usenode.proto.set_valueif that internal flag is needed.hasattr(element, "set_value")andhasattr(element, "click")now returnTruefor everyElementthat does not already define those methods, because a stub callable is returned instead of raisingAttributeError. Helpers that probed those names as capability checks will now getAppTestErrorat call time.Implements wiki queue P0.2 in the AppTest issue verification and PR queue.
GitHub Issue Link (if applicable)
N/A
Testing Plan
lib/tests/streamlit/testing/element_tree_test.py— pagination inspects;set_value/clickraiseAppTestError; typed widgets withoutclick()point testers atset_value();node.proto.set_valuestill readableContribution License Agreement
By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.
Made with Cursor