Skip to content
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

Mount the whole playwright tests directory so that pwtests-ui can watch for changes without a restart. #3908

Merged
merged 2 commits into from
May 28, 2024

Conversation

jyasskin
Copy link
Collaborator

This doesn't break pwtests-update; is there anything else I should check?

@dlaliberte
Copy link
Collaborator

James @jcscottiii might have some input regarding the docker configuration.

Copy link
Collaborator

@jcscottiii jcscottiii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🎉

Some background:

  • When this was initially set up, there were permission issues when the playwright container would write the screenshots. The container and host had different users and permissions. As a result, if one wrote something the other could not read it. This led to us creating the same user inside the container from the host and copying things instead of mounting them. We may be able to get rid of this final copy in the Dockerfile if you are not seeing any permission issues now. Could you remove that final COPY in the Dockerfile and try to run the playwright tests?

@jyasskin
Copy link
Collaborator Author

Just removing that final COPY fails because wait-for-app.sh and playwright.config.cjs aren't present anymore. Expanding the mount to include all of ./ mounts it over the installed node_modules directory, so that the playwright executable isn't present anymore. I've uploaded a change that narrows the copy to just the 2 files that are needed, but I'm not certain that the efficiency gain in avoiding the extra copies is worth confusing ourselves when we add a third file in the root directory that needs to be present in the container. What do you think?

@jyasskin jyasskin merged commit 870909d into GoogleChrome:main May 28, 2024
7 checks passed
@jyasskin jyasskin deleted the mount-playwright-tests branch May 28, 2024 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants