feat: 🎸 Use snapshots or OCR for E2E tests when comparing terminal #2639
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Some points of consideration
- Terminal output may look different if using docker-based infrastructure vs. AWS-based infrastructure. Not sure if there are any test cases that need docker.
- If using snapshots, it's possible that some people's personal shells may affect terminal output
I haven't used docker for anything yet in DC tests but my assumption was we would only be testing against targets that were spun up somewhere which I assumed would be EC2 instances when using SSH targets. What do you generally connect to for the docker based tests, localhost?
Yup, the assumption was that the output from logging into an EC2 instance would scroll past it, which may not be true in all cases. |
On docker-based tests, we spin up a separate container to use as a target (https://hub.docker.com/r/linuxserver/openssh-server). |
|
@ZedLi interesting PR. Upon reading the PR description I'm wondering if a short video or screenshots would help review this one? I'm curious about the discovery that led to this work, is there a jira ticket for more context? |
I'm not sure video or screenshots would help as it's just the test running and waiting properly so it looks the same between them. The context is really just solving the issue in this TODO, I'll add a little bit more context in the description though! |
|
@ZedLi Thanks for adding context to the description. RE: I had to use poll as it turns out the documentation is a bit misleading... Can you link the docs you are referencing? |
| @@ -7,7 +7,13 @@ import { mergeTests } from '@playwright/test'; | |||
| import { electronTest } from './electronTest.js'; | |||
| import { authenticateTest } from './authenticateTest.js'; | |||
| import { test as envTest } from '../../global-setup.js'; | |||
| import { tesseractTest } from './tesseractTest.js'; | |||
|
|
|||
| // The last item in the list will override any previous ones in a fixture name collision | |||
There was a problem hiding this comment.
Interesting work.
RE: override any previous ones
To clarify this commented line, any variables that could possibly exist in a test suite (e.g. envTest), but also exist in tesseractTest(being last) will be overridden?
There was a problem hiding this comment.
Yeah, so if there's any use variables from these fixtures that have the same name, the one that appears later in this list will end up taking priority and override any previous ones.
There was a problem hiding this comment.
Gotcha. Thanks. I wonder if that comment could be a bit clearer? Wdyt?
There was a problem hiding this comment.
What do you think it should say? This was more of a general comment to remind people how merging works if for some reason you are overriding
There was a problem hiding this comment.
I read a bit of the playwright docs to get more context, thanks for sharing those. I'd prob comment something like:
// Please be aware that defining like fixtures across different test envs
// in Playwright can cause naming collisions
// To solve for this, any like declarations that exist in the last test in this list item
// will override all declarations, if those like declarations exist in previous test envs
There was a problem hiding this comment.
Non-blocking suggestion, though. Good work 👍
There was a problem hiding this comment.
I'll update this comment to be a bit more descriptive to explain the merging behavior
I was just referencing the playwright docs for toHaveScreenshot where they mention:
which is not quite accurate. |
| await use(worker); | ||
| await worker.terminate(); | ||
| }, | ||
| { scope: 'worker' }, |
There was a problem hiding this comment.
how is this scope being used?
There was a problem hiding this comment.
Here's the doc for worker scopes
ZedLi
force-pushed
the
use-snapshots-for-e2e-tests
branch
from
d4f8b04 to
7b92e70
Compare
Description
When reviewing this PR, please take a look at both commits and test both. The goal of this PR is to resolve this TODO as sometimes we want to interact with our integrated terminal. The main problem is that the terminal is built using a
canvaselement which makes it difficult to test with playwright as it's basically an image with no DOM elements to interact with. The particular problem we're trying to solve in this PR is knowing when the desktop client properly finishes making the SSH connection and is "done" when using the integrated terminal.The first commit adds a golden screenshot to compare to what we expect a properly connected SSH session to look like. This of course can be quite brittle due to many factors (size of window, if the EC2 instance has new updates, timestamps, etc) but allowing a little bit of difference seems to work still. It's hard to say how brittle it is as the target machines can change so I'm curious if it works well for others. I had to use
pollas it turns out the documentation is a bit misleading andtoHaveScreenshotdoesn't actually wait until they match and just compares screenshots after they have been stabilized, which is usually before the SSH connection finishes.The second commit adds
tesseract.jsand takes a different approach by using an OCR package to interpret the screenshot of the canvas element that holds our terminal. I use a phrase that I expect to see every time we connect to an EC2 instance and have it keep checking to see if it appears. The main downside is the setup of the package adds a decent amount of time (~5s on my mac but I set it up so it only runs once per worker and should be reused) and a few more seconds to actually recognize the image in the test. In theory this should be less brittle but may also be unnecessary as it's decently slower.How to Test
Check out each commit and try running
yarn desktoporyarn desktop:devChecklist
[ ] I have added before and after screenshots for UI changes[ ] I have added JSON response output for API changes[ ] I have added steps to reproduce and test for bug fixes in the description[ ] My changes generate no new warnings