feat(lab): scaffolds a basic UI test #1581
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
apps/laboratory/package.json
Outdated
| @@ -5,7 +5,11 @@ | |||
| "scripts": { | |||
| "dev:laboratory": "next dev", | |||
| "build:laboratory": "next build", | |||
| "lint": "eslint . --ext .js,.jsx,.ts,.tsx" | |||
| "lint": "eslint . --ext .js,.jsx,.ts,.tsx", | |||
| "playwright:start": "turbo run build; npm run dev:laboratory", | |||
There was a problem hiding this comment.
Since I don't have turbo installed, I can't run npm run playwright:test locally. We should make a npm run turbo-build -> turbo run build script and call that here instead
There was a problem hiding this comment.
I think I can remove the turbo run build from the command. Would that address your concern?
There was a problem hiding this comment.
You can run npm run build instead
There was a problem hiding this comment.
@arein the script is broken now; you can't run npm run build in the lab directory; that script does not exist. turbo run build worked because it recurses into the parent directory.
There was a problem hiding this comment.
You are right actually. For everything to work properly / build packages etc. Set up and commands should be ran from the monorepo root. Similarly something like npm run laboratory should be ran from there. Doing otherwise i.e. running from individual packages could have unexpected side effects probably
There was a problem hiding this comment.
This sets everything up nicely, but does not include new approach we discussed with Jon, Sven and others for v3 testing.
Due to fast moving pace and changes, we'd need to spend substantial amount of time on re-updating tests if they are based on granular checks i.e. like you check for All Wallets.
Road forward with v3 that will keep us moving fast while providing good feedback on breaking something is through playwrights visual diff tests
More context here https://playwright.dev/docs/test-snapshots
TL;DR these are extremely cheap to set up dev time wise for existing and upcoming features while providing really good feedback on breaking changes i.e. if something is off, pixels will be off as well (missing notification. modal not open etc.). Updating these will be easy as well, i.e. if something changed and it was expected, we just update reference screenshot.
@arein Could you please switch from using "All Wallets" detection in the test, to one using screenshot diffing i.e. snap of open modal.
| use: { ...devices['Desktop Firefox'] } | ||
| } | ||
|
|
||
| /* Test against mobile viewports. */ |
There was a problem hiding this comment.
Either needs a TODO or to be removed if unused
apps/laboratory/package.json
Outdated
| @@ -5,7 +5,11 @@ | |||
| "scripts": { | |||
| "dev:laboratory": "next dev", | |||
| "build:laboratory": "next build", | |||
| "lint": "eslint . --ext .js,.jsx,.ts,.tsx" | |||
| "lint": "eslint . --ext .js,.jsx,.ts,.tsx", | |||
| "playwright:start": "turbo run build; npm run dev:laboratory", | |||
There was a problem hiding this comment.
You are right actually. For everything to work properly / build packages etc. Set up and commands should be ran from the monorepo root. Similarly something like npm run laboratory should be ran from there. Doing otherwise i.e. running from individual packages could have unexpected side effects probably
What's actually requested?
I'm scaffolding everything here @0xasimetriq specifically to avoid comments like these. I am not actually testing anything here. The |
| @@ -5,7 +5,12 @@ | |||
| "scripts": { | |||
| "dev:laboratory": "next dev", | |||
| "build:laboratory": "next build", | |||
| "lint": "eslint . --ext .js,.jsx,.ts,.tsx" | |||
| "build:all": "turbo run build", | |||
There was a problem hiding this comment.
Turborepo and monorepo wide commands should not be ran from here. There is global npm run build command that does exactly this in the root, can that be reused? It also sets up clean builds properly for all packages.
You will likely need to move playwright commands and test dependencies to monorepo root as well and just target tests inside laboratory folder.
Taking full advantage of Playwright will involve a lot more than screenshots; every edge case should have a test case and needing to update something trivial like "All Wallets" will be the least of your concerns when changing stuff around. |
So for Q1, right now there seems to be a For Q2, we should scaffold screenshot diffing flow in this pr as well, a simple single screenshot check for now will be sufficient. Idea is to have devs be able to start with this scaffold right away right, so without them needing to add this logic in. Otherwise this scaffold is likely incomplete imho. |
Yes, but remember this is ui product so majority of issues will bubble up to ui, which is why screenshot diffing is great here. I.e if you set an option to disable metamask, all you have to do is screenshot that and it should not be there, if it is, then we made a regression. This applies to almost every config / action in web3modal, if something is wrong it will result in a different visual to what is expected (even text in error notifications for example) |
Yes, but I feel like this is the easy way out and is easier to mess up. There's 1 command that will reset all of the screenshots. So say you have a new feature that adds a new wallet to the list of wallets, and you reset all of the screenshots. You get your new wallet, but you also removed MetaMask without realizing it. Simply the screenshot checks will not detect this problem. Now since the screenshots are committed and reviewed, hopefully someone will notice that the one called "MetaMask exists" does indeed still contain a MetaMask. I guess my point is that it's easier to mess up an image than an explicit code removal/modifiction of the MetaMask test case. I haven't used these snapshots before, so maybe I'll see the light :D |
Breaking Changes
N/A
Changes
Next: #1582
Associated Issues
closes #...