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

Fix devcontainer configuration #1256

Merged
merged 1 commit into from
Aug 25, 2024

Conversation

radanskoric
Copy link
Contributor

@radanskoric radanskoric commented May 6, 2024

I wanted to try some changes to turbo source on to get myself started quickly I tried using the devcontainer setup but it didn't work. So instead I spent the time debugging that issue.

The devcontainer was no longer working since chrome version 115 because location of installation files changed. See comment on: https://chromedriver.chromium.org/downloads

However we don't need to build it ourselves since Microsoft maintains docker images with playwright preinstalled with chrome, firefox and webkit.

This means that we can simplify the setup a lot while still keeping the goal of allowing people to get an environment in which they can run tests by simply starting a devcontainer or running a Github codespace.

I've just tested this now by launching a github codespace from this here branch on my fork. After waiting a bit, I was able to just write yarn test and it worked. (Strangely there were a few tests that failed. They were flaky, the runner re-ran them and they passed and where flagged as flaky by the runner)

@radanskoric
Copy link
Contributor Author

@robzolkos is there anything else needed to merge this?

@robzolkos
Copy link

@robzolkos is there anything else needed to merge this?

Jut the attention of a maintainer. This cleans things up a fair bit.

Copy link
Collaborator

@brunoprietog brunoprietog left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution, love the simplicity! Just a couple of minor comments

.devcontainer/devcontainer.json Outdated Show resolved Hide resolved
.devcontainer/devcontainer.json Show resolved Hide resolved
@radanskoric
Copy link
Contributor Author

@brunoprietog I've updated the PR.

I also had to slightly bump @playwright/test to 1.30.0 because we already had playwright-core at both 1.28.0 and 1.30.0 through different dependency chains, causing issues. Unit test were trying to run with 1.28 and browser tests with 1.30 which is a problem when the docker image has just one playwright build.

Playwright should probably also be bumped to latest, 1.45.3. I'm happy to do that as well but I concluded it's best left for next PR, to limit the scope of this one.

@brunoprietog
Copy link
Collaborator

Happy to see a PR updating Playwright! Maybe it will be helpful in reducing some flaky tests

The devcontainer was no longer working since chrome
version 115 because location of installation files changed.
See comment on: https://chromedriver.chromium.org/downloads

However we don't need to build it ourselves since Microsoft
maintains docker images with playwright preinstalled with
chrome, firefox and webkit.

This means that we can simplify the setup a lot while still
keeping the goal of allowing people to get an environment
in which they can run tests by simply starting a devcontainer
or running a Github codespace.

I also had to slightly bump @playwright/test to 1.30.0 because
we already had both 1.28.0 and 1.30.0 through different
dependency chains, causing issues. Unit test were trying to run
with 1.28 and browser tests with 1.30 which is a problem when
our docker image has just one playwright build.
Copy link
Collaborator

@brunoprietog brunoprietog left a comment

Choose a reason for hiding this comment

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

Thanks @radanskoric!

@brunoprietog brunoprietog merged commit f48b0de into hotwired:main Aug 25, 2024
1 check passed
@radanskoric radanskoric deleted the fix-devcontainer branch August 26, 2024 09:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants