[feat] Add external URL runner in e2e playwright#13678
[feat] Add external URL runner in e2e playwright#13678sfc-gh-bnisco merged 2 commits intodevelopfrom
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. |
sfc-gh-bnisco
added
change:feature
Graphite App
This stack of pull requests is managed by Graphite. Learn more about stacking. |
✅ PR preview is ready!
|
sfc-gh-bnisco
force-pushed
the
01-22-_feat_add_external_url_runner_in_e2e_playwright
branch
from
9e9dfa9 to
24b4488
Compare
📉 Frontend coverage change detectedThe frontend unit test (vitest) coverage has decreased by 0.0000%
✅ Coverage change is within normal range. |
sfc-gh-bnisco
force-pushed
the
01-22-_feat_add_external_url_runner_in_e2e_playwright
branch
from
24b4488 to
07b02c8
Compare
sfc-gh-bnisco
added
the
ai-review
github-actions
bot
removed
the
ai-review
SummaryThis PR adds the ability to run E2E Playwright tests against an externally hosted Streamlit app URL instead of spinning up a local server. It introduces:
Code QualityThe implementation is clean and follows existing patterns in the codebase. Key observations: Positives:
Issues:
Test Coverage
Backwards CompatibilityThis change is fully backwards compatible:
Security & Risk
Recommendations
# In conftest.py
config.addinivalue_line(
"markers",
"iframe_test: mark test as compatible with external app execution mode",
)
# In skip fixture
if external_app_url and request.node.get_closest_marker("iframe_test") is None:
VerdictCHANGES REQUESTED: The implementation is solid and the feature is useful, but the marker naming should be changed from This is an automated AI review using |
There was a problem hiding this comment.
Pull request overview
This PR adds support for running e2e playwright tests against externally hosted Streamlit apps instead of requiring a local server. This enables testing against production-like environments or remote deployments.
Changes:
- Added CLI options
--external-app-urland--browser-state-path(with corresponding environment variables) to configure external app testing - Created
app_base_urlfixture that returns either the external URL or local URL based on configuration - Modified
app_serverfixture to skip server startup when external mode is active - Added auto-skip logic to restrict external mode to tests marked with
@pytest.mark.iframeTest - Updated
test_no_console_errorsto useapp_base_urland marked it as external-compatible
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| e2e_playwright/conftest.py | Added fixtures and CLI options for external app URL and browser state, modified app_server to skip startup in external mode, added iframeTest marker and auto-skip logic |
| e2e_playwright/mega_tester_app_test.py | Updated test_no_console_errors to use app_base_url fixture and marked as iframeTest compatible |
Comments suppressed due to low confidence (1)
e2e_playwright/conftest.py:408
- The
static_appfixture at line 393 hardcodeshttp://localhost:{app_port}and doesn't support external app mode. Similar to theappfixture, this should either be updated to support external mode or there should be documentation explaining why it's excluded. Consider whether this fixture should useapp_base_urlor if static apps inherently can't work with external mode.
@pytest.fixture
def static_app(
page: Page,
app_port: int,
request: pytest.FixtureRequest,
) -> Page:
"""Fixture that opens the app."""
query_param = request.node.get_closest_marker("query_param")
query_string = query_param.args[0] if query_param else ""
# Indicate this is a StaticPage
page.__class__ = StaticPage
page.goto(f"http://localhost:{app_port}/{query_string}")
start_capture_traces(page)
wait_for_app_loaded(page)
return page
07b02c8 to
212927b
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
Comments suppressed due to low confidence (1)
e2e_playwright/conftest.py:408
- The
static_appfixture uses hardcodedhttp://localhost:{app_port}instead of the newapp_base_urlfixture. This will prevent tests using this fixture from working with external apps. Consider updating this fixture to accept and useapp_base_urlparameter.
@pytest.fixture
def static_app(
page: Page,
app_port: int,
request: pytest.FixtureRequest,
) -> Page:
"""Fixture that opens the app."""
query_param = request.node.get_closest_marker("query_param")
query_string = query_param.args[0] if query_param else ""
# Indicate this is a StaticPage
page.__class__ = StaticPage
page.goto(f"http://localhost:{app_port}/{query_string}")
start_capture_traces(page)
wait_for_app_loaded(page)
return page
212927b to
6ca20fe
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
e2e_playwright/conftest.py:454
- The
static_appfixture still usesapp_portdirectly to construct URLs. For consistency and to support external app testing, consider updating it to useapp_base_urlinstead, similar to how theappfixture was updated. This would allowstatic_appto work with externally hosted apps.
@pytest.fixture
def static_app(
page: Page,
app_port: int,
request: pytest.FixtureRequest,
) -> Page:
"""Fixture that opens the app."""
query_param = request.node.get_closest_marker("query_param")
query_string = query_param.args[0] if query_param else ""
# Indicate this is a StaticPage
page.__class__ = StaticPage
page.goto(f"http://localhost:{app_port}/{query_string}")
start_capture_traces(page)
wait_for_app_loaded(page)
return page
7a22365 to
2099055
Compare
sfc-gh-bnisco
force-pushed
the
01-22-_feat_add_external_url_runner_in_e2e_playwright
branch
from
2099055 to
e0ba6f5
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
e2e_playwright/conftest.py:578
- The
static_app,app_with_query_params, andiframed_appfixtures still hardcodehttp://localhost:{app_port}URLs instead of using theapp_base_urlfixture. This means they won't work with external app URLs. If these fixtures should also support external mode, they should be updated to useapp_base_url. If they're not meant to work in external mode, tests using them should be automatically skipped in external mode (similar to how tests without theexternal_testmarker are skipped).
@pytest.fixture
def static_app(
page: Page,
app_port: int,
request: pytest.FixtureRequest,
) -> Page:
"""Fixture that opens the app."""
query_param = request.node.get_closest_marker("query_param")
query_string = query_param.args[0] if query_param else ""
# Indicate this is a StaticPage
page.__class__ = StaticPage
page.goto(f"http://localhost:{app_port}/{query_string}")
start_capture_traces(page)
wait_for_app_loaded(page)
return page
@pytest.fixture
def app_with_query_params(
page: Page, app_port: int, request: pytest.FixtureRequest
) -> tuple[Page, dict[str, Any]]:
"""Fixture that opens the app with additional query parameters.
The query parameters are passed as a dictionary in the 'param' key of the request.
"""
query_params = request.param
query_string = parse.urlencode(query_params, doseq=True)
url = f"http://localhost:{app_port}/?{query_string}"
page.goto(url)
wait_for_app_loaded(page)
return page, query_params
e0ba6f5 to
5b7e6e1
Compare
sfc-gh-bnisco
force-pushed
the
01-22-_feat_add_external_url_runner_in_e2e_playwright
branch
from
5b7e6e1 to
d6871d4
Compare
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
SummaryAdds external app execution support for Playwright e2e tests via new CLI/env options, fixtures to skip local server startup, optional host-page/iframe targeting inputs, and Playwright storage state support, plus documentation updates. Code QualityOverall structure is clear and consistent with existing e2e test infrastructure. There are a couple of URL normalization behaviors that can unintentionally alter user-provided external URLs:
Test CoverageNo new unit or e2e tests were added. This is understandable for infrastructure-level changes, but the external URL behaviors (especially path handling) are untested. A minimal, opt-in test (or a doc-only verified manual step) would help validate external app and host URL flows. Backwards CompatibilityLocal (default) e2e behavior is preserved. External mode introduces URL normalization that may break apps hosted on path-sensitive routes (e.g., Security & RiskNo direct security concerns identified. Main risk is unintended routing changes for externally hosted apps as noted above. Recommendations
VerdictCHANGES_REQUESTED: Please address the external URL path normalization risks before merge to avoid breaking path-sensitive deployments. This is an automated AI review using |
lukasmasuch
left a comment
lukasmasuch
left a comment
There was a problem hiding this comment.
LGTM 👍 But maybe take another look at the path normalization as suggested by ai-review.
sfc-gh-bnisco
force-pushed
the
01-22-_feat_add_external_url_runner_in_e2e_playwright
branch
from
d6871d4 to
bf9998a
Compare
| def _build_app_url(base_url: str, *, fragment: str | None) -> str: | ||
| """Build an app navigation URL from ``base_url`` and an optional fragment. | ||
|
|
||
| This preserves the user-provided path (including any trailing slash) to | ||
| avoid altering routing semantics for externally hosted apps. | ||
|
|
||
| The only normalization we do is mapping an empty path to ``"/"`` so that | ||
| ``https://example.com#foo`` and ``https://example.com/#foo`` both navigate | ||
| to the root path with the given fragment. | ||
| """ | ||
| split = parse.urlsplit(base_url) | ||
| path = _normalize_empty_path(split.path) | ||
|
|
||
| if fragment is not None: | ||
| frag = fragment.lstrip("#") | ||
| return parse.urlunsplit((split.scheme, split.netloc, path, split.query, frag)) | ||
|
|
||
| return parse.urlunsplit((split.scheme, split.netloc, path, split.query, "")) |
There was a problem hiding this comment.
The _build_app_url function discards any fragment/hash that exists in the base_url when fragment parameter is None. This will break external app URLs that include a fragment.
What breaks: If a user provides --external-app-url https://example.com/app#section1 and a test doesn't use @pytest.mark.app_hash, the #section1 fragment is lost because line 220 sets the fragment to an empty string "" instead of preserving split.fragment.
Fix:
if fragment is not None:
frag = fragment.lstrip("#")
return parse.urlunsplit((split.scheme, split.netloc, path, split.query, frag))
return parse.urlunsplit((split.scheme, split.netloc, path, split.query, split.fragment))| def _build_app_url(base_url: str, *, fragment: str | None) -> str: | |
| """Build an app navigation URL from ``base_url`` and an optional fragment. | |
| This preserves the user-provided path (including any trailing slash) to | |
| avoid altering routing semantics for externally hosted apps. | |
| The only normalization we do is mapping an empty path to ``"/"`` so that | |
| ``https://example.com#foo`` and ``https://example.com/#foo`` both navigate | |
| to the root path with the given fragment. | |
| """ | |
| split = parse.urlsplit(base_url) | |
| path = _normalize_empty_path(split.path) | |
| if fragment is not None: | |
| frag = fragment.lstrip("#") | |
| return parse.urlunsplit((split.scheme, split.netloc, path, split.query, frag)) | |
| return parse.urlunsplit((split.scheme, split.netloc, path, split.query, "")) | |
| def _build_app_url(base_url: str, *, fragment: str | None) -> str: | |
| """Build an app navigation URL from ``base_url`` and an optional fragment. | |
| This preserves the user-provided path (including any trailing slash) to | |
| avoid altering routing semantics for externally hosted apps. | |
| The only normalization we do is mapping an empty path to ``"/"`` so that | |
| ``https://example.com#foo`` and ``https://example.com/#foo`` both navigate | |
| to the root path with the given fragment. | |
| """ | |
| split = parse.urlsplit(base_url) | |
| path = _normalize_empty_path(split.path) | |
| if fragment is not None: | |
| frag = fragment.lstrip("#") | |
| return parse.urlunsplit((split.scheme, split.netloc, path, split.query, frag)) | |
| return parse.urlunsplit((split.scheme, split.netloc, path, split.query, split.fragment)) | |
Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.
sfc-gh-bnisco
force-pushed
the
01-22-_feat_add_external_url_runner_in_e2e_playwright
branch
from
bf9998a to
8a999a0
Compare
Merge activity
|
sfc-gh-bnisco
deleted the
01-22-_feat_add_external_url_runner_in_e2e_playwright
branch
Describe your changes
Added support for running Playwright E2E tests against externally hosted Streamlit apps (instead of starting a local app server).
--external-app-url(orSTREAMLIT_E2E_EXTERNAL_APP_URL) to target an already-running app URL--external-host-url(orSTREAMLIT_E2E_EXTERNAL_HOST_URL) for cases where the app is embedded in a host page--external-iframe-selector(orSTREAMLIT_E2E_EXTERNAL_IFRAME_SELECTOR, defaultiframe) to locate the embedded app iframe--browser-state-path(orSTREAMLIT_E2E_BROWSER_STATE_PATH) to load a Playwright storage state JSON fileexternal_testmarker (@pytest.mark.external_test) for tests that are compatible with external mode.external_testare skipped; if no tests are marked, pytest fails early with a clear usage error to avoid silently skipping the entire suite.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.