-
-
Notifications
You must be signed in to change notification settings - Fork 160
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
Upgrade Yarn to 3.6.4 to fix issue with Node 20.x (part 1) #1406
Upgrade Yarn to 3.6.4 to fix issue with Node 20.x (part 1) #1406
Conversation
Hello. Thanks for opening a PR on Exercism 🙂 We ask that all changes to Exercism are discussed on our Community Forum before being opened on GitHub. To enforce this, we automatically close all PRs that are submitted. That doesn't mean your PR is rejected but that we want the initial discussion about it to happen on our forum where a wide range of key contributors across the Exercism ecosystem can weigh in. You can use this link to copy this into a new topic on the forum. If we decide the PR is appropriate, we'll reopen it and continue with it, so please don't delete your local branch. If you're interested in learning more about this auto-responder, please read this blog post. Note: If this PR has been pre-approved, please link back to this PR on the forum thread and a maintainer or staff member will reopen it. |
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 left some comments. Before we can merge this, I'd like to verify that this works with the test runner (do we need to upgrade there). Let me know if you need help with that
.github/workflows/ci.js.yml
Outdated
|
||
impersonate: | ||
# This job tries to run tests for the 'hello-world' exercise, but | ||
# simulating what would happen if a student runs the tests standalone. | ||
# We do this by removing all project files in the root. | ||
|
||
runs-on: ubuntu-latest | ||
|
||
steps: | ||
- uses: actions/checkout@8e5e7e5ab8b370d6c329ec480221332ada57f0ab | ||
- name: Use Node.js 20.x | ||
uses: actions/setup-node@64ed1c7eab4cce3362f8c340dee64e5eaeef8f7c | ||
with: | ||
node-version: 20.x | ||
|
||
- name: Remove root project files | ||
run: rm -f package.json yarn.lock .yarnrc.yml | ||
|
||
- name: Move solution file so that tests pass | ||
working-directory: exercises/practice/hello-world | ||
run: | | ||
rm -f hello-world.ts | ||
cp ./.meta/proof.ci.ts hello-world.ts | ||
|
||
- name: Install project dependencies | ||
working-directory: exercises/practice/hello-world | ||
run: yarn install --no-immutable | ||
|
||
- name: Run tests | ||
working-directory: exercises/practice/hello-world | ||
run: yarn test |
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 you extract this to a separate PR? It is related to this PR, but it is something else IMHO.
.github/workflows/pr.ci.js.yml
Outdated
|
||
impersonate: | ||
# This job tries to run tests for the 'hello-world' exercise, but | ||
# simulating what would happen if a student runs the tests standalone. | ||
# We do this by removing all project files in the root. | ||
|
||
runs-on: ubuntu-latest | ||
|
||
steps: | ||
- uses: actions/checkout@8e5e7e5ab8b370d6c329ec480221332ada57f0ab | ||
- name: Use Node.js 20.x | ||
uses: actions/setup-node@64ed1c7eab4cce3362f8c340dee64e5eaeef8f7c | ||
with: | ||
node-version: 20.x | ||
|
||
- name: Remove root project files | ||
run: rm -f package.json yarn.lock .yarnrc.yml | ||
|
||
- name: Move solution file so that tests pass | ||
working-directory: exercises/practice/hello-world | ||
run: | | ||
rm -f hello-world.ts | ||
cp ./.meta/proof.ci.ts hello-world.ts | ||
|
||
- name: Install project dependencies | ||
working-directory: exercises/practice/hello-world | ||
run: yarn install --no-immutable | ||
|
||
- name: Run tests | ||
working-directory: exercises/practice/hello-world | ||
run: yarn test |
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 you extract this to a separate PR? It is related to this PR, but it is something else IMHO.
… would" This reverts commit 2516ba4.
I've moved the new CI step to another PR: #1409
As I mentioned on the forum, I tried to make it work earlier this week but got a weird script error - but I admit I didn't have time to investigate. To save you time, let me take another stab at it first. Would we want to update the test runner to Node 20 if we also update Yarn there? |
@ErikSchierboom Ok, I've finally been able to sort my script issue. When I run the Output: bin_run_sh_output.log I've tried this both with Node 18 and Node 20 and I get the same result for both (a success). I think the test runner works fine because it's not using PnP like is used when a student downloads an exercise locally. Would you like me to try to update Yarn to 3.6.4 in the test runner (and perhaps Node to 20.x)? In any case, the test runner is working correctly as-is so it could be updated later if preferred. |
Yeah that would be nice! |
@ErikSchierboom PR for the test runner update: exercism/typescript-test-runner#65 |
The TypeScript track currently uses Yarn 3.6.0. However, there is an issue with this version of Yarn when used on recent versions of Node 20.x, as reported here.
I believe it could be a manifestation of the bug fixed by this commit in Yarn 3.6.3.
This PR updates Yarn to 3.6.4 for all exercises. It also adds a CI step to run a test on the
hello-world
exercise as the student would see it, so that this issue can be detected in the future.Note: the full diff for the changes is actually too big for GitHub, unfortunately, so I cannot create the PR with all changes. The second PR is here: #1407
The steps to merge all this would be:
main
(I am submitting this following @iHiD 's request on the Discord server. I will post on the forum when both PRs are ready.)