Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate disconnect and disabled from cypress to playwright #8732

Merged
merged 12 commits into from
Jun 4, 2024

Conversation

sfc-gh-wihuang
Copy link
Contributor

Describe your changes

GitHub Issue Link (if applicable)

Testing Plan

  • Explanation of why no additional tests are needed
  • Unit Tests (JS and/or Python)
  • E2E Tests
  • Any manual testing needed?

Contribution License Agreement

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

@willhuang1997 willhuang1997 changed the title Attempt at migrating disconnect to playwright Remove disconnect from e2e tests May 22, 2024
@willhuang1997 willhuang1997 marked this pull request as ready for review May 22, 2024 22:15
@sfc-gh-wihuang sfc-gh-wihuang changed the title Remove disconnect from e2e tests Migrate disconnect and disabled from cypress to playwright May 23, 2024
Copy link
Collaborator

@LukasMasuch LukasMasuch left a comment

Choose a reason for hiding this comment

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

Overall, this looks good. But I think these two tests can be combined to one since they are both testing disconnection. I would probably combine it into something like websocket_disconnect test file to make it a bit more obvious what it is testing. Maybe it's even enough to just have the test_disconnecting_disables_widgets_correctly since it seems to include the other logic.

@willhuang1997
Copy link
Collaborator

Overall, this looks good. But I think these two tests can be combined to one since they are both testing disconnection. I would probably combine it into something like websocket_disconnect test file to make it a bit more obvious what it is testing. Maybe it's even enough to just have the test_disconnecting_disables_widgets_correctly since it seems to include the other logic.

Sounds good. Will do that. Thanks

Copy link
Collaborator

@LukasMasuch LukasMasuch left a comment

Choose a reason for hiding this comment

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

LGTM 👍

expect(app.get_by_test_id("stMarkdown").first).to_contain_text("Value 1: 25")

expect(app.get_by_test_id("stConnectionStatus")).not_to_be_visible()
app.evaluate("window.streamlitDebug.shutdownRuntime()")
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: maybe add a comment here that calling this will disconnect the WebSocket connection -> which should put all elements into disabled mode.

@sfc-gh-wihuang sfc-gh-wihuang enabled auto-merge (squash) June 3, 2024 18:13
@sfc-gh-wihuang sfc-gh-wihuang merged commit 65c0a6b into develop Jun 4, 2024
33 of 34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants