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.

Sign up for GitHub

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

Jump to bottom

Remove unneeded install command. #3427

Merged
merged 2 commits into from
Oct 19, 2023
Merged

Remove unneeded install command. #3427

merged 2 commits into from
Oct 19, 2023

Conversation

jrobbins
Copy link
Collaborator

The tests run for me without this command. On chat, you said that it was not needed.

@jrobbins jrobbins requested a review from jcscottiii October 19, 2023 00:50
@dlaliberte
Copy link
Collaborator

I see that the Playwright CI tests are failing. Looks like maybe this install command is relevant in that context? If so, can we move the install command to only be used in the CI test?

@jcscottiii
Update playwright Dockerfile version. Make dependabot track Dockerfil…
fa13196 
…e updates (#3429)

* Update playwright Dockerfile version

Without this commit: `npm install` runs and installs the version in package.json which was 1.39 while the preinstalled browsers targeted playwright 1.37.


Now that we are relying on the already installed libraries and browsers, we can get rid of the npm install in the Dockerfile too. 

TODO: Modify [dependabot](https://github.com/GoogleChrome/chromium-dashboard/blob/main/.github/dependabot.yml) to update the docker image. This should help prevent package.json playwright from outpacing the docker file image.

* Update dependabot.yml to track Dockerfile updates

* try

* final
@jcscottiii
Copy link
Collaborator

I did some poking around in #3427. The docker image needed to be updated to match the version in package.json. Additionally, I added a dependabot config to update the Dockerfile. This should help prevent the Docker image from getting too out of sync from the package.json dependency.

@jrobbins jrobbins merged commit 6aeec90 into main Oct 19, 2023
@jrobbins jrobbins deleted the 20231018-remove-step-from-playwright-dockerfile branch October 19, 2023 17:09
@dlaliberte
Copy link
Collaborator

We used to have a comment in the "//pwtests" pseudo command like this (copied from bikeshed):
"//pwtests": "The version of the docker playwright image used here must exactly match the versions of @playwright/test in devDependencies and the Github Actions configuration (in playwright.yml). The version of playwright used in devDependencies should not include the ^ prefix, so it doesn't auto-upgrade and get out of sync.",

I had thought that we made changes to avoid need for this manual updating, but it seems like that must not be the case. To avoid this problem in the future, maybe we should add this "//pwtests" commend back in.

@dlaliberte
Copy link
Collaborator

We used to have a comment in the "//pwtests" pseudo command like this (copied from bikeshed).

BTW, in case people missed it, in my last PR I added a "//pwtests" comment to the packages/playwright/packages.json:

        "//pwtests": "The version of playwright used here in devDependencies:@playwright/test 
    must be the same as the docker image version used in Dockerfile, 
    so the '^' prefix should not be included to avoid auto-upgrade of playwright 
    which would then be out of sync.",

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jcscottiii jcscottiii jcscottiii approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jrobbins @dlaliberte @jcscottiii