This repository has been archived by the owner on Feb 22, 2023. It is now read-only.
Fix/e2e tests #1640
Merged
Fix/e2e tests #1640
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Storybook and Tailwind configuration previews: Ready Storybook: https://wordpress.github.io/openverse-frontend/_preview/1640 Please note that GitHub pages takes a little time to deploy newly pushed code, if the links above don't work or you see old versions, wait 5 minutes and try again. You can check the GitHub pages deployment action list to see the current status of the deployments. |
openverse-bot
added
💻 aspect: code
|
Size Change: -4.71 kB (0%) Total Size: 1.44 MB
ℹ️ View Unchanged
|
bin/playwright.sh
Outdated
| @@ -9,8 +9,8 @@ export PLAYWRIGHT_ARGS=$@ | |||
| export PLAYWRIGHT_VERSION=$(version) | |||
| export TEST_COMMAND=${TEST_COMMAND:-test:playwright:local} | |||
|
|
|||
| echo Running Playwright v$PLAYWRIGHT_VERSION as $USER_ID with Playwright arguments $PLAYWRIGHT_ARGS | |||
| cp -r test/locales/* src/locales | |||
There was a problem hiding this comment.
I wonder if it's worth doing this in the Dockerfile.playwright instead of in here.
There was a problem hiding this comment.
I removed it because we actually need the matching locale file in other tests, not only the Playwright ones.
When the valid-locales.json is not copied in setup-env step in other tests such as Storybook, the app fails to build. Also, simply copying the valid-locales.json is not enough because the app requires all of the translation json listed in valid-locales.json to build.
|
|
||
| docker-compose -f docker-compose.playwright.yml down | ||
| docker-compose -f docker-compose.playwright.yml up --build --force-recreate --exit-code-from playwright --remove-orphans |
There was a problem hiding this comment.
So was it the docker-compose down that was causing this not to fail because the script continued to the dc down command after the failure of the test run? If so, an alternative fix (assuming the down is necessary) would be to add set -x to the top of the script so that any command failure causes the script to exit with the failure exit code.
There was a problem hiding this comment.
Well, if you remove the dc down line, the tests start to fail again. But I'm not very clear as to what exactly happens. I'll try your suggestion.
There was a problem hiding this comment.
If that is the case then I think it is what I said. The default behavior with shell scripts is not to bail out the entire script if a command fails unless the e option is set.
I had remembered it wrong, btw, it should be set -e. set -x prints the commands as they're run. e bails on the script if any error occurs. https://linuxcommand.org/lc3_man_pages/seth.html
There was a problem hiding this comment.
It worked! Thank you :)
How did you see that the Docker container was still running after tests in GitHub actions, by the way?
There was a problem hiding this comment.
How did you see that the Docker container was still running after tests in GitHub actions, by the way?
What do you mean?
There was a problem hiding this comment.
I was going to suggest using docker-compose run --rm ... instead but glad to see it's working now! :)
There was a problem hiding this comment.
I don't remember why but dc run did not work for me when I tried it when originally developing the dockerized Playwright tests. There was something strange about it where it would never end the containers, IIRC.
There was a problem hiding this comment.
What do you mean?
The dc down line was added to solve the problem where a Playwright container wouldn't stop, right? I have never known that it didn't stop in the CI. How did you understand that that was, indeed, happening?
There was a problem hiding this comment.
I think @krysal added it (IIRC) upon noticing that the containers weren't stopping locally. I don't think it matters if the containers continue to run in CI, as far as I know.
There was a problem hiding this comment.
I think @krysal added it (IIRC) upon noticing that the containers weren't stopping locally.
That is correct. The machines running the GH actions are a big black box, so that would have been much more difficult to identify, though locally it was a coincidence 🙂
5 tasks
dhruvkb
approved these changes
Aug 11, 2022
|
I was going to ask why are we marking all the tests to run in parallel mode? I did that for the slowest tests in my PR, those that triggered warnings after the test execution. If we want to do that we can set it up in the Playwright config file ( |
|
@krysal, I first marked the e2e tests that were triggering the 'slow test' warning, and then just went ahead and added that line in every file. |
7 tasks
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes
Fixes #1637 by @obulat
Description
This PR fixes the failing Playwright visual regression tests, and makes the CI fail if Playwright tests fail.
The changes introduced in #1559 to stop the Docker container after Playwright tests caused the CI not to fail even if the Playwright container exits with status
1.As can be seen in this run's log, Playwright tests started to fail again after the following line from
bin/playwright.shwas removed:docker-compose -f docker-compose.playwright.yml downI've also updated the snapshots that were failing.
Testing Instructions
Look at the CI logs. The most recent ones are passing, but the run before that should fail because one of the snapshots didn't match.
Checklist
Update index.md).main) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin