Playwright tests & CI #69
Conversation
There was a problem hiding this comment.
@vincanger awesome work, this will take open-saas to new levels of professionalism and give us much more confidence when working on it :)!
I did a review of everything but the Playwright tests themselves -> once we take care of most of the stuff, I can also review those into more details. I was instead focusing on how it is all orchestrated.
Some general stuff:
Normally, you would run CI on every PR and on main branch. That way we get CI checks done when for the PR, so we know all is fine before merging into main, and then also on main, again, just to ensure main is all good after the merging and something did go wrong.
This is also how we do it for wasp repo.
You mentioned testing Stripe but for that you need a different type of auth, not username & pass, and how you could handle that using different git branches.
In general, I would say that is not the usual approach -> you don't want to get entangled into maintaining multiple branches at the same time -> instead normally you would aim for having these other versions as some kind of example apps in the repo, that you run these tests on. So maybe there would be "examples" dir at the top level of the repo, and you would test this stuff there. Not saying that doing something with branches can't be done, maybe it can and maybe it is potentially interesting, but I wouldn't consider it as a go-to option.
Btw, we are likely to switch open saas to using email&pass very soon because it has a good support now (with 0.12) for running in dev with no setup at all, so maybe that will help a bit for Stripe testing?
As for giving them these e2e tests or not, setup via Github Actions -> my first instinct is to let them have it, by default, since it is nice, does the tests for you, and we give them instructions how to delete it if they don't like it.
ALthough retag-commit is problematic, we don't want them to have that one hm!
If they use this template via wasp new, then it is easy, but if they clone the Github repo as a template, then I don't know how we can do this properly.
I am tempted to say we allow using it only via wasp new! It gives us more control, we can do what we want here in repo and wasp new can delete some files upon download, or download only some files (although they don't get git repo immediately).
|
okie dokie @Martinsos ready for another review. here's a rundown:
|
e2e-tests/setupDatabaseName.sh
Outdated
|
|
||
| # Construct the Postgres URL. We assume the host is localhost since it's a local Docker environment | ||
| # and the default Postgres port is 5432. Adjust if necessary. | ||
| DATABASE_URL="postgresql://postgresWaspDevUser:postgresWaspDevPass@localhost:5432/${DB_NAME}" |
There was a problem hiding this comment.
Right, we also assume the username and pass to match those provided by wasp db start.
Co-authored-by: Martin Šošić <[email protected]>
Co-authored-by: Martin Šošić <[email protected]>
Co-authored-by: Martin Šošić <[email protected]>
Co-authored-by: Martin Šošić <[email protected]>
Co-authored-by: Martin Šošić <[email protected]>
…saas into playwright-tests
| path: ~/.npm | ||
| key: node-modules-${{ hashFiles('app/package-lock.json') }}-${{ hashFiles('e2e-tests/package-lock.json') }}-${{ runner.os }}-wasp${{ env.WASP_VERSION }}-node${{ steps.setup-node.outputs.node-version }} | ||
| restore-keys: | | ||
| node-modules-${{ runner.os }}- |
There was a problem hiding this comment.
Well, almost there :D. Since this is your restore-key, and it makes sense, you want to include OS version, you will also want to update your actual key to have runner.os as the first thing after node-modules, otherwise this will never match!
There was a problem hiding this comment.
ok I wasn't aware of that, I thought you suggested it that way because it would match any combination of keys, but it makes sense that it is looking for a substring of that key.
There was a problem hiding this comment.
Actually it is looking for a prefix I think! So this way it matches the prefix.
| HOBBY_SUBSCRIPTION_PRICE_ID: ${{ secrets.HOBBY_SUBSCRIPTION_PRICE_ID }} | ||
| PRO_SUBSCRIPTION_PRICE_ID: ${{ secrets.PRO_SUBSCRIPTION_PRICE_ID }} | ||
| CREDITS_PRICE_ID: ${{ secrets.CREDITS_PRICE_ID }} | ||
| SKIP_EMAIL_VERIFICATION_IN_DEV: true |
There was a problem hiding this comment.
We set this env var in .env.server.example as well, FYI
There was a problem hiding this comment.
I will leave it there in case the user changes their local env so that CI tests don't fail
| "e2e:playwright": "DEBUG=pw:webserver npx playwright test", | ||
| "_comment-on-local:e2e:cleanup-stripe": "NOTE: because we are running the stripe webhook listener in the background, we want to make sure we kill the previous processes before starting a new one.", | ||
| "local:e2e:cleanup-stripe": "PID=$(ps -ef | grep 'stripe listen' | grep -v grep | awk '{print $2}') || true && kill -9 $PID || true", | ||
| "local:e2e:start-stripe": "stripe listen --forward-to localhost:3001/stripe-webhook &", |
There was a problem hiding this comment.
Did we mention in the README that you need to install the Stripe CLI locally to get the e2e tests working?
There was a problem hiding this comment.
@infomiho no we don't. good idea. i've gone ahead and added it.
|
Good job 👍 I'd maybe add a note in the README about how to install Stripe CLI locally if you want to run the e2e tests locally. |
There was a problem hiding this comment.
@vincanger we want a long way here, but I think we are good now! Great job, we fixed a lot of stuff!
I approved but, but pls check this last comment: https://github.com/wasp-lang/open-saas/pull/69/files#r1572247170 .
Also, now that we improved stuff here, we should take that and apply the improvements on e2e tests we have in wasp repo. You could coordinate with @infomiho around that. I recommend doing that as soon as possible, because otherwise you will forget a lot about what we did here and it will become quite harder.
Adding some basic playwright tests, as well as a couple github workflows.
Questions:
deployed-versionbranch (the branch which gets deployed to opensaas.sh)? this way, we can test new features before updating the template onmain, as well as the app that gets deployed to opensaas.sh without adding a lot of complexity to the template the users initially get. if users DO want to opt in to tests/CI, we can instruct them on how to do so in the docs.mainships with username and password auth only to start, as it doesn't require any ENV VARS and allows the user to get started right away, but it is not compatible with Stripe checkout. Therefore, if we want extensive testing of the template, we should probably go the feature branch/staging environment route...