8000 [feat] Add external URL runner in e2e playwright by sfc-gh-bnisco · Pull Request #13678 · streamlit/streamlit · GitHub
[go: up one dir, main page]

Skip to content

Conversation

@sfc-gh-bnisco
Copy link
Collaborator
@sfc-gh-bnisco sfc-gh-bnisco commented Jan 22, 2026

Describe your changes

Added support for running E2E tests against externally hosted Streamlit apps by introducing two new command-line options:

  1. --external-app-url: Allows tests to run against an external app URL instead of localhost
  2. --browser-state-path: Enables using a Playwright storage state JSON file for authentication

Added a new iframeTest marker to identify tests that are compatible with external app execution mode, and implemented a fixture to skip non-compatible tests when running in external mode.

Created helper functions to retrieve configuration from either command-line options or environment variables, and added fixtures to manage the app base URL and browser state.

Testing Plan

  • The usual CI pipeline validates the existing tests.
  • The changes have been manually tested by running the mega tester app test with the external app URL option.

Contribution License Agreement

By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.

@snyk-io
Copy link
Contributor
snyk-io bot commented Jan 22, 2026

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

Status Scanner Critical High Medium 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.

@sfc-gh-bnisco sfc-gh-bnisco added change:feature PR contains new feature or enhancement implementation impact:internal PR changes only affect internal code labels Jan 22, 2026 — with Graphite App
Copy link
Collaborator Author

@github-actions
Copy link
Contributor
github-actions bot commented Jan 22, 2026

✅ PR preview is ready!

Name Link
📦 Wheel file https://core-previews.s3-us-west-2.amazonaws.com/pr-13678/streamlit-1.53.1-py3-none-any.whl
📦 @streamlit/component-v2-lib Download from artifacts
🕹️ Preview app pr-13678.streamlit.app (☁️ Deploy here if not accessible)

@sfc-gh-bnisco sfc-gh-bnisco force-pushed the 01-22-_feat_add_external_url_runner_in_e2e_playwright branch from 9e9dfa9 to 24b4488 Compare January 23, 2026 21:05
@github-actions
Copy link
Contributor
github-actions bot commented Jan 23, 2026

📉 Frontend coverage change detected

The frontend unit test (vitest) coverage has decreased by 0.0000%

  • Current PR: 86.2600% (13560 lines, 1862 missed)
  • Latest develop: 86.2600% (13560 lines, 1862 missed)

✅ Coverage change is within normal range.

📊 View detailed coverage comparison

@sfc-gh-bnisco sfc-gh-bnisco force-pushed the 01-22-_feat_add_external_url_runner_in_e2e_playwright branch from 24b4488 to 07b02c8 Compare January 26, 2026 21:03
@sfc-gh-bnisco sfc-gh-bnisco requested a review from Copilot January 27, 2026 00:38
@sfc-gh-bnisco sfc-gh-bnisco added the ai-review If applied to PR or issue will run AI review workflow label Jan 27, 2026
@github-actions github-actions bot removed the ai-review If applied to PR or issue will run AI review workflow label Jan 27, 2026
@github-actions
Copy link
Contributor

Summary

This 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:

  • Two new CLI options: --external-app-url and --browser-state-path
  • Corresponding environment variable support: STREAMLIT_E2E_EXTERNAL_APP_URL and STREAMLIT_E2E_BROWSER_STATE_PATH
  • A new iframeTest marker to designate tests compatible with external app execution mode
  • An app_base_url fixture for tests that need to work in both local and external modes
  • Automatic skipping of non-marked tests when running in external app mode

Code Quality

The implementation is clean and follows existing patterns in the codebase. Key observations:

Positives:

  • Type annotations are properly used throughout, including the cast on line 133 with an explanatory comment
  • The _get_config_option_or_env helper function is well-structured and provides a clean abstraction
  • The browser_state_path fixture properly validates the file exists before returning (lines 311-313)
  • The skip mechanism using pytest.skip() is appropriate and follows best practices

Issues:

  1. Marker naming inconsistency (e2e_playwright/conftest.py:104): The new marker iframeTest uses camelCase, which is inconsistent with existing markers in the codebase:

    • no_perf (snake_case)
    • app_hash (snake_case)

    Per PEP 8 and the project's Python guidelines, this should be iframe_test.

  2. Incomplete PR description: The PR template sections for description, testing plan, and changes are empty. This makes it difficult to understand the intent and use cases for this feature.

Test Coverage

  • The change modifies test_no_console_errors in mega_tester_app_test.py to demonstrate the new pattern by:

    • Adding the @pytest.mark.iframeTest marker
    • Switching from app_port: int to app_base_url: str parameter
    • Using goto_app(page, app_base_url) instead of a hardcoded localhost URL
  • Concern: There are no unit tests for the new _get_config_option_or_env helper function. While this is a simple utility, adding a test would ensure the fallback logic between CLI options and environment variables works correctly.

  • The autouse fixture skip_non_iframe_tests_in_external_mode is not directly tested, though its behavior can be verified by running tests with the external URL option.

Backwards Compatibility

This change is fully backwards compatible:

  • All existing tests continue to work without modification
  • The external app mode is entirely opt-in via CLI options or environment variables
  • When not using external mode, the behavior is identical to before

Security & Risk

  • Low risk: The browser_state_path fixture validates the file exists before use, which prevents potential errors
  • The feature is additive and isolated behind explicit opt-in flags
  • No sensitive data handling concerns identified

Recommendations

  1. Rename the marker for consistency - Change iframeTest to iframe_test in both conftest.py (lines 103-105, 351) and mega_tester_app_test.py (line 65):
# 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:
  1. Fill in the PR description - Add details about:

    • The use case for this feature (e.g., testing against deployed apps)
    • How to use the new CLI options
    • The testing plan/verification performed
  2. Consider adding a unit test for _get_config_option_or_env to verify:

    • CLI option takes precedence over environment variable
    • Environment variable is used when CLI option is not provided
    • Returns None when neither is set
  3. Minor documentation suggestion - Consider adding a brief comment or docstring update in the module explaining the external app testing workflow for future maintainers.

Verdict

CHANGES REQUESTED: The implementation is solid and the feature is useful, but the marker naming should be changed from iframeTest to iframe_test for consistency with existing markers and Python naming conventions. Additionally, the PR description should be filled in to explain the feature and its use cases.


This is an automated AI review using opus-4.5-thinking. Please verify the feedback and use your judgment.

@github-actions github-actions bot added the do-not-merge PR is blocked from merging label Jan 27, 2026
Copy link
Contributor
Copilot AI left a comment

Choose a reason for hiding this comment

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

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-url and --browser-state-path (with corresponding environment variables) to configure external app testing
  • Created app_base_url fixture that returns either the external URL or local URL based on configuration
  • Modified app_server fixture 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_errors to use app_base_url and 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_app fixture at line 393 hardcodes http://localhost:{app_port} and doesn't support external app mode. Similar to the app fixture, this should either be updated to support external mode or there should be documentation explaining why it's excluded. Consider whether this fixture should use app_base_url or 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

@sfc-gh-bnisco sfc-gh-bnisco force-pushed the 01-22-_feat_add_external_url_runner_in_e2e_playwright branch from 07b02c8 to 212927b Compare January 27, 2026 00:48
@sfc-gh-bnisco sfc-gh-bnisco removed the do-not-merge PR is blocked from merging label Jan 27, 2026
@sfc-gh-bnisco sfc-gh-bnisco requested a review from Copilot January 27, 2026 01:01
Copy link
Contributor
Copilot AI left a comment

Choose a reason for hiding this comment

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

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_app fixture uses hardcoded http://localhost:{app_port} instead of the new app_base_url fixture. This will prevent tests using this fixture from working with external apps. Consider updating this fixture to accept and use app_base_url parameter.
@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

@sfc-gh-bnisco sfc-gh-bnisco force-pushed the 01-22-_feat_add_external_url_runner_in_e2e_playwright branch from 212927b to 6ca20fe Compare January 27, 2026 19:33
@sfc-gh-bnisco sfc-gh-bnisco requested a review from Copilot January 27, 2026 19:34
Copy link
Contributor
Copilot AI left a comment

Choose a reason for hiding this comment

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

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_app fixture still uses app_port directly to construct URLs. For consistency and to support external app testing, consider updating it to use app_base_url instead, similar to how the app fixture was updated. This would allow static_app to 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

@sfc-gh-bnisco sfc-gh-bnisco force-pushed the 01-22-_feat_add_external_url_runner_in_e2e_playwright branch from 6ca20fe to 852ba18 Compare January 28, 2026 04:01
@sfc-gh-bnisco sfc-gh-bnisco force-pushed the 01-22-_feat_add_external_url_runner_in_e2e_playwright branch from 852ba18 to 7a22365 Compare January 28, 2026 04:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

change:feature PR contains new feature or enhancement implementation impact:internal PR changes only affect internal code security-assessment-completed Security assessment has been completed for PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

0