Skip to content

[fix] Raise AppTestError for unsupported AppTest interactions - #16837

Merged
lukasmasuch merged 3 commits into
developfrom
fix/apptest-p0-2-unsupported-interaction
Sep 7, 2026
Merged

lukasmasuch merged 3 commits into
developfrom
fix/apptest-p0-2-unsupported-interaction

Conversation

@lukasmasuch

@lukasmasuch lukasmasuch commented Sep 5, 2026 •

Copy link
Copy Markdown
Collaborator

Describe your changes

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.

The protobuf set_value field on UnknownElement widgets is no longer readable as an attribute; use node.proto.set_value if that internal flag is needed.

hasattr(element, "set_value") and hasattr(element, "click") now return True for every Element that does not already define those methods, because a stub callable is returned instead of raising AttributeError. Helpers that probed those names as capability checks will now get AppTestError at 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 / click raise AppTestError; typed widgets without click() point testers at set_value(); node.proto.set_value still readable
  • E2E tests: not applicable (AppTest-only)

Contribution 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

Copilot AI balanced review requested due to automatic review settings September 5, 2026 10:48
@lukasmasuch lukasmasuch added change:bugfix PR contains bug fix implementation impact:users PR changes affect end users feature:app-testing Related to `AppTest` testing framework labels Sep 5, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@snyk-io

snyk-io Bot commented Sep 5, 2026 •

Copy link
Copy Markdown
Contributor

✅ Snyk checks have passed. No issues have been found so far.

Status Scan Engine Image Critical Image High Image Medium Image Low Total (0)
✅ Open Source Security 0 0 0 0 0 issues
✅ Licenses 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@github-actions

github-actions Bot commented Sep 5, 2026 •

Copy link
Copy Markdown
Contributor

✅ PR preview is ready!

Name Link
📦 Wheel file https://core-previews.s3-us-west-2.amazonaws.com/pr-16837/streamlit-1.63.0-py3-none-any.whl
📦 @streamlit/component-v2-lib Download from artifacts

@lukasmasuch

Copy link
Copy Markdown
Collaborator Author

Added Agent Docs

@lukasmasuch lukasmasuch added the ai-review If applied to PR or issue will run AI review workflow label Sep 5, 2026
@greptile-apps

greptile-apps Bot commented Sep 5, 2026 •

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR improves AppTest diagnostics for unsupported element interactions.

  • Intercepts unsupported set_value() and click() calls and raises an actionable AppTestError.
  • Preserves typed interaction methods and direct access to internal protobuf fields through node.proto.
  • Adds coverage for pagination, markdown, and typed widgets without click() support.

Confidence Score: 5/5

The PR appears safe to merge with no outstanding correctness or repository-rule issues.

Supported typed methods continue to resolve normally, unsupported interactions now fail with the intended AppTestError, and the motivating pagination behavior and protobuf access path are covered by tests.

Important Files Changed

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

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

Reviewed by Cursor Bugbot for commit 0da0fcb. Configure here.

Comment thread lib/streamlit/testing/v1/element_tree.py
@github-actions github-actions Bot removed the ai-review If applied to PR or issue will run AI review workflow label Sep 5, 2026

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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") and hasattr(element, "click") return True for every element, including Markdown. Nothing in lib/streamlit/testing/ duck-types on those names today; user helpers that branched on hasattr would move from “skip” to “raise at call time.”
  • Reading element.set_value as a proto bool on UnknownElement widgets no longer works (a callable is returned instead). Testers who need that internal flag can still use node.proto.set_value. set_value is not a public st.pagination parameter.

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 unsupported set_value / click
    • lib/streamlit/testing/v1/errors.py: AppTestError docstring only
    • lib/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 AppTest to 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_value is a proto field; click previously raised AttributeError.
  • 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

  1. 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.
  2. Optional follow-up: add a typed Pagination wrapper 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.
  3. 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.

Comment thread lib/streamlit/testing/v1/element_tree.py Outdated
Comment thread lib/tests/streamlit/testing/element_tree_test.py Outdated
Comment thread lib/streamlit/testing/v1/errors.py Outdated
Comment thread lib/streamlit/testing/v1/element_tree.py Outdated
Comment thread lib/streamlit/testing/v1/element_tree.py Outdated
Comment thread lib/tests/streamlit/testing/element_tree_test.py Outdated
Inspectable-only elements still point testers at Playwright; widgets
that already support set_value() now get a matching diagnostic.
@lukasmasuch lukasmasuch changed the title [fix] Raise AppTestError for inspectable-only widgets [fix] Raise AppTestError for unsupported AppTest interactions Sep 5, 2026
@lukasmasuch lukasmasuch added ai-final-review If applied to PR or issue will run AI review workflow with the latest generation of AI models. and removed feature:app-testing Related to `AppTest` testing framework labels Sep 5, 2026
@github-actions github-actions Bot removed the ai-final-review If applied to PR or issue will run AI review workflow with the latest generation of AI models. label Sep 5, 2026

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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:

  1. Proto set_value is no longer readable as an attribute on unimplemented widgets. Documented in the PR body; use node.proto.set_value. In practice this only affects st.pagination today. Reading that internal flag from a test has no plausible use.
  2. hasattr(element, "click") / hasattr(element, "set_value") now return True for every Element that does not already define those methods, because the stub is returned instead of AttributeError. Duck-typed helpers that probed these names as capability checks can now enter the stub and get AppTestError. Same as hasattr on a disabled button's real click. 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

  1. Optional: reword the non-widget AppTestError so it does not claim unmodeled widgets "are not interactive," and mention at.session_state for keyed widgets (see inline on element_tree.py).
  2. Optional: note the hasattr behavior change in the PR body next to the proto-attribute migration.
  3. Optional: pin the key suffix / guidance sentence and node.proto.set_value in the new tests.
  4. Follow-up (out of scope): a typed AppTest wrapper for st.pagination (and st.link_button now that it supports on_click) would close the gap this error makes discoverable. A one-sentence note in lib/streamlit/.agents/skills/developing-with-streamlit/references/testing.md next to the disabled-widget AppTestError note 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.

Comment thread lib/streamlit/testing/v1/element_tree.py Outdated
Comment thread lib/tests/streamlit/testing/element_tree_test.py Outdated
Comment thread lib/streamlit/testing/v1/element_tree.py
Comment thread lib/streamlit/testing/v1/element_tree.py Outdated
Comment thread lib/streamlit/testing/v1/element_tree.py

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Auto-approved by AI final review (workflow run).

Comment thread lib/streamlit/testing/v1/element_tree.py
@lukasmasuch
lukasmasuch merged commit 429a1a5 into develop Sep 7, 2026
45 of 47 checks passed
@lukasmasuch
lukasmasuch deleted the fix/apptest-p0-2-unsupported-interaction branch September 7, 2026 10:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

change:bugfix PR contains bug fix implementation impact:users PR changes affect end users

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants