-
Notifications
You must be signed in to change notification settings - Fork 1k
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
E2E Smoke test #4497
E2E Smoke test #4497
Conversation
Setup build:testproject script to copy from fixtures
eb0758e
to
f19dd75
Compare
Correct smoke-test path
Update timeout
@@ -18,26 +17,5 @@ console.log({ | |||
|
|||
core.setOutput('test_project_path', test_project_path) | |||
|
|||
// See https://github.com/actions/toolkit/tree/main/packages/io#cpmv. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is now taken care of by yarn build:test-project
run: | | ||
yarn install --immutable | ||
yarn build:clean || echo "Project already clean" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also done as part of the test project generator
@jtoar, @thedavidprice ready for review. We can build on top of this, as we see a need for more coverage! Only problem I see is that the CLI checks take decades to complete on windows... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dac09 this is awesome! I really like how the fixtures let us couple all the server-starting logic with the test itself. 🚀
I'm still getting my head around what's going on, but I left a few initial comments. One more I had that wasn't specific to any particular line of code: since the processes are running in parallel and both are piping to stdout, do you think we can do something to make the output a little easier to follow, mostly for debugging purposes? I'll suggest an idea if I come up with one.
@dac09 mostly just throwing this out there, but what if we made the test project a playwright fixture too? That way playwright could handle everything. |
Nice @dac09 I'll take this for a review + spin on Monday! |
…test * 'main' of github.com:redwoodjs/redwood: (23 commits) Netlify client getToken fix when GoTrue client refreshes JWT (redwoodjs#4539) Update dependency @supabase/supabase-js to v1.30.4 (redwoodjs#4536) Envelop: Don't use useImmediateIntrospection as it causes auth bug (redwoodjs#4538) Update dependency react-hook-form to v7.27.1 (redwoodjs#4521) try increasing timeout for flaky test (redwoodjs#4526) Update dependency stacktracey to v2.1.8 (redwoodjs#4519) Update dependency msw to v0.38.1 (redwoodjs#4525) Update dependency eslint-config-prettier to v8.4.0 (redwoodjs#4522) Provide a Revised Runtime Error Page (redwoodjs#4167) update yarn.lock v0.46.0 Update dependency esbuild to v0.14.23 (redwoodjs#4518) Fix Storybook build args (redwoodjs#4455) Update dependency react-helmet-async to v1.2.3 (redwoodjs#4502) Bump url-parse in /__fixtures__/example-todo-main-with-errors (redwoodjs#4511) Bump url-parse from 1.5.1 to 1.5.7 in /__fixtures__/example-todo-main (redwoodjs#4512) Update dependency fastify to v3.27.2 (redwoodjs#4516) Uncomment role checks (redwoodjs#4476) Update dependency zx to v5.1.0 (redwoodjs#4505) Update dependency firebase to v9.6.7 (redwoodjs#4514) ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Woah, DC, you really did a great job stitching together the elements here. Much much better than what I originally had in my mind.
I haven't run this locally yet — just stepped through code and looked through CI runs. Only question since I'm not familiar with Playwright --> have you tested a failure on rw dev
or rw serve
to make sure the Smoke Test fails the CI workflow?
Left some comments mostly about Telemetry related needs.
Thanks so much!
tasks/test-project/test-project
Outdated
{ | ||
title: 'Updating ports in redwood.toml...', | ||
task: () => { | ||
const REDWOOD_TOML_PATH = path.join( | ||
OUTPUT_PROJECT_PATH, | ||
'redwood.toml' | ||
) | ||
const redwoodToml = fs.readFileSync(REDWOOD_TOML_PATH).toString() | ||
let newRedwoodToml = redwoodToml | ||
|
||
newRedwoodToml = newRedwoodToml.replace( | ||
/\port = 8910/, | ||
'port = "${WEB_DEV_PORT:8910}"' | ||
) | ||
|
||
newRedwoodToml = newRedwoodToml.replace( | ||
/\port = 8911/, | ||
'port = "${API_DEV_PORT:8911}"' | ||
) | ||
|
||
fs.writeFileSync(REDWOOD_TOML_PATH, newRedwoodToml) | ||
}, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this go in the Playwright fixtures instead of here, where it will be included in every built Test-project (for both CI and local dev)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ummmm in theory... yes I could put it in playwright, but then I'd have to check if the toml is already modified.
However, this change, as you can see has the fallback to the usual 8910 and 8911. So normal test projects continue to work, even without the env vars defined
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but then I'd have to check if the toml is already modified
Not sure what this means as the toml will be know 'cause it's coming from the fixture?
Not a dealbreaker. Just feels like a weird artifact for all the local dev usage. If you decide it's best to leave here, then maybe it deserves a comment to explain why it's there in the test project?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a comment explaining why. I think given that this change has no impact unless you're actually setting the env var, I prefer to keep it in the test project generator, mainly so that the change is applied once.
Playwright fixtures can run multiple times, on different workers, which is why we'd have to add a check to see if the toml is already modified, which feels a little unnecessary to me.
@dac09 Just wanted to make sure you saw all my misc comments throughout from yesterday. |
@jtoar, @thedavidprice - all comments addressed from both of you - thank you for the in-depth reviews! I tried enabling the visual test, but I'm afraid with the fonts varying ever so slightly, the snapshots don't match up, so best not to have a potentially flakey test in there |
Smoke tests for
rw dev
andrw serve
This PR does the following:
__fixtures__/test-project
. This can be disabled (i.e. you can rebuild, if you run it with--no-copy-from-fixtures
To run the smoke test locally, you need to do the following
a) Create a test project e.g.
yarn build:test-project ~/Experiments/copy-guy-3 --ts --verbose --no-canary
b) and run
PROJECT_PATH="~/Experiments/copy-guy-3" yarn smoke-test --headed
.Note: if you get a failure first time running it because of the playwright dependencies, please make a note of the command playwright tells you to run! I have install deps as part of the
smoke-test
script, but can't verify on my machine because I have the dependencies.Other Stuff:
I also tried adding some visual snapshot tests, to make sure css/tailwind/etc are all working as expected - however I had to disable them as the snapshots I produced in gitpod seem to differ from the github runner ones, possibly due to fonts.
Mac and windows work though.
Maybe someone on linux could help with this? Not a requirement to merge the PR
Related to #4500
Closes #4259