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

UI tests with Playwright #413

Merged
merged 41 commits into from
Jun 10, 2024
Merged

Conversation

blakerosenthal
Copy link
Contributor

Added some tests for the UI with Playwright! Here's a couple decisions I made that could use another gut check or two:

  • I chose to spin up an actual uvicorn server for the API rather than mock those endpoints. I also chose not to mock the UI endpoints for the sake of actually testing browser integration.
  • I set different ports for the API and the UI so as to not conflict with other servers that might be running on the same machine.

Questions:

  • What's happening with my auth setup? Setting a token either in the browser header or the panel cookies/headers doesn't seem to bypass the main auth page.
  • Where should I look for better CSS locators? Playwright codegen offers some wacky identifiers. @pierrotsmnrd maybe you could point me to the right files for this?
  • Will this work with GH Actions? Who knows; we'll find out!

Copy link
Member

@pmeier pmeier left a comment

Choose a reason for hiding this comment

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

Thanks Blake for the PR. One thing that might simplify the setup quite a bit is the usage of our CLI entrypoint directly. Meaning, instead of starting the API and UI yourself, just run

@app.command(help="Start the web UI.")
def ui(

directly. I'm not 100% sure this works, but it should.

environment-dev.yml Outdated Show resolved Hide resolved
ragna/deploy/__init__.py Outdated Show resolved Hide resolved
tests/deploy/ui/test_ui.py Outdated Show resolved Hide resolved
tests/deploy/ui/test_ui.py Outdated Show resolved Hide resolved
tests/deploy/ui/test_ui.py Outdated Show resolved Hide resolved
tests/deploy/ui/test_ui.py Outdated Show resolved Hide resolved
tests/deploy/ui/test_ui.py Outdated Show resolved Hide resolved
tests/deploy/ui/test_ui.py Outdated Show resolved Hide resolved
tests/deploy/ui/test_ui.py Outdated Show resolved Hide resolved
@blakerosenthal
Copy link
Contributor Author

... just run

@app.command(help="Start the web UI.")
def ui(

directly. I'm not 100% sure this works, but it should.

I will try this and see what happens. I think I initially thought you couldn't snag a typer function like that but maybe I'm wrong.

@blakerosenthal
Copy link
Contributor Author

@pmeier (cc @pierrotsmnrd)
Running into a bit of a snag running the two servers in the test env. The tests running in CI are experiencing timeouts waiting for the server to come up, but locally there are some different errors with the various streams getting closed too early. I must not be managing the various sub processes correctly: pytest_output.txt

Then, separate issue, clicking on the "start chat" button gives me a pydantic validation error that only shows up in playwright (line 182).

I wasn't seeing the issues with the server in previous versions of this PR where I spun up each server separately.

Final thought: given that we're expecting the bokeh/fast-api integration soon-ish, does it make sense to punt on some of these issues until that is merged? At least a couple of these roadblocks might solve themselves.

@pmeier
Copy link
Member

pmeier commented May 23, 2024

the tests running in CI are experiencing timeouts waiting for the server to come up

You've set the timeout to only 5 seconds, which is a little to low. A timeout is there to prevent the test suite to run indefinitely when something goes wrong. Since you have a proper check on when the server is started, I suggest you increase the timeout to something like 30 seconds. Recall that CI machines are small so it might take them 2-3 times to get this server up compared to your local setup.

but locally there are some different errors with the various streams getting closed too early.

I've seen these error before when using Chrome / Chromium. That seems to be a bug in panel or bokeh. However, I've never seen an impact of these on the actual functionality of Ragna. So not sure what is going wrong in the background, but I think we can just ignore them. If we have time at some point, we can report this to panel. Try switching the browser to Firefox and see if the errors persist.

With increasing the timeout to 30 seconds everything runs smoothly locally for me. I'll push this change to your branch to see if CI is happy as well.

tests/deploy/utils.py Outdated Show resolved Hide resolved
@blakerosenthal
Copy link
Contributor Author

@pmeier This is ready for another look.

Video from failing playwright tests is now uploaded as an artifact. Waiting half a second before clicking "start conversation" seems to resolve all the failures I was seeing. There's probably some backend thing where the system isn't fully ready for the click yet even though the button exists. I don't feel like this needs to be investigated urgently since real users won't be clicking buttons in less than half a second after the dialog appears.

start_chat_button = page.get_by_role("button", name="Start Conversation")
expect(start_chat_button).to_be_visible()
time.sleep(0.5) # hack while waiting for button to be fully clickable
start_chat_button.click(delay=5)

Copy link
Member

@pmeier pmeier left a comment

Choose a reason for hiding this comment

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

Good work Blake! A few minor comments, but this already looks pretty good.

.github/workflows/test.yml Show resolved Hide resolved
tests/deploy/api/test_batch_endpoints.py Outdated Show resolved Hide resolved
tests/deploy/utils.py Outdated Show resolved Hide resolved
tests/deploy/api/test_components.py Outdated Show resolved Hide resolved
tests/deploy/api/test_e2e.py Outdated Show resolved Hide resolved
tests/deploy/ui/test_ui.py Outdated Show resolved Hide resolved
tests/deploy/ui/test_ui.py Outdated Show resolved Hide resolved
tests/deploy/ui/test_ui.py Outdated Show resolved Hide resolved
tests/deploy/ui/test_ui.py Outdated Show resolved Hide resolved
tests/deploy/ui/test_ui.py Outdated Show resolved Hide resolved
@blakerosenthal
Copy link
Contributor Author

@pmeier Done :)

.github/workflows/test.yml Outdated Show resolved Hide resolved
.github/workflows/test.yml Show resolved Hide resolved
tests/deploy/ui/test_ui.py Outdated Show resolved Hide resolved
tests/deploy/ui/test_ui.py Outdated Show resolved Hide resolved
@blakerosenthal blakerosenthal merged commit 8ea62a3 into Quansight:main Jun 10, 2024
1 check was pending
@blakerosenthal blakerosenthal deleted the ui-tests branch June 10, 2024 16:59
blakerosenthal added a commit that referenced this pull request Jul 17, 2024
Co-authored-by: Philip Meier <github.pmeier@posteo.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants