Create E2E tests for common AppNode scenarios not particularly tested #12818
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!
|
48d1cd6 to
50e7ec0
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR adds end-to-end tests for AppNode hierarchy scenarios to improve test coverage of edge cases in the AppNode implementation, which is undergoing significant changes.
Key Changes:
- New E2E test suite covering four AppNode hierarchy scenarios: element swapping, element insertion, long computations with stale elements, and placeholder updates
- Test app implementing the scenarios to be tested
Reviewed Changes
Copilot reviewed 2 out of 5 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| e2e_playwright/appnode_hierarchy_test.py | E2E tests verifying AppNode hierarchy behavior for element swapping, insertion, stale element handling, and placeholder updates |
| e2e_playwright/appnode_hierarchy.py | Test application providing interactive scenarios for AppNode hierarchy testing |
| _select_mode(app, "insert_between") | ||
|
|
||
| expect( | ||
| app.get_by_test_id("stMarkdown").filter(has_text="between") |
There was a problem hiding this comment.
The filter has_text='between' will match both 'between' and 'insert_between' text. This assertion may pass unexpectedly if 'insert_between' is present. Use an exact text match or a more specific filter to avoid false positives.
There was a problem hiding this comment.
This is not true because insert_between is not a markdown element.
|
|
||
| get_button(app, "run long compute").click() | ||
| # Wait one second to make sure stale elements are there (and styled correctly) | ||
| app.wait_for_timeout(1000) |
There was a problem hiding this comment.
The Streamlit E2E Tests guide explicitly prohibits using wait_for_timeout and requires using the wait_until utility instead. Replace app.wait_for_timeout(1000) with wait_until to wait for the specific condition of stale elements appearing with the data-stale attribute.
Spotted by Graphite Agent (based on custom rule: E2E Playwright Tests)
Is this helpful? React 👍 or 👎 to let us know.
There was a problem hiding this comment.
This review is technically correct, but I am not sure it is possible to wait until the animation to stale ends. Open to ideas here.
There was a problem hiding this comment.
If we were getting really strict, we could update our frontend logic that could set a class based on its animation state (eg: {stale-animating-in,stale,stale-animating-out}). Using those, we could write an assertion in the test code that waits for everything to have stale classes. That would probably be the most deterministic and not rely on timeout logic. But, probably not necessary at the moment.
09805fb to
89ad1a4
Compare
89ad1a4 to
6b854af
Compare
|
|
||
| get_button(app, "run long compute").click() | ||
| # Wait one second to make sure stale elements are there (and styled correctly) | ||
| app.wait_for_timeout(1000) |
There was a problem hiding this comment.
If we were getting really strict, we could update our frontend logic that could set a class based on its animation state (eg: {stale-animating-in,stale,stale-animating-out}). Using those, we could write an assertion in the test code that waits for everything to have stale classes. That would probably be the most deterministic and not rely on timeout logic. But, probably not necessary at the moment.
Describe your changes
Added end-to-end tests for AppNode hierarchy edge cases to ensure proper element rendering and state management. The tests verify:
The implementation includes a test app with multiple scenarios and corresponding Playwright tests that validate the expected behavior in each case.
Testing Plan
Contribution License Agreement
By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.