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

Extend Node version test coverage #1843

Merged
merged 4 commits into from
Nov 8, 2024

Conversation

joshmgross
Copy link
Member

#1841 could have been caught in unit tests.

As long as we have users of the toolkit with deprecated actions (Node 16) and running with the default Node version on hosted runners (Node 18), we should avoid breaking the toolkit by utilizing Node 20 features.

In the future, we may want to be more clear about what versions of Node we support in the toolkit.

@joshmgross joshmgross requested a review from a team as a code owner October 4, 2024 20:39
@joshmgross
Copy link
Member Author

This is failing as expected due to #1841, so I've confirmed that this would catch the reference error 🎉

ReferenceError: crypto is not defined
https://github.com/actions/toolkit/actions/runs/11186675222/job/31102183371?pr=1843

CI should be passing once #1842 is merged.

@joshmgross
Copy link
Member Author

Removed Node 16 as the Artifact tests were not passing

FAIL packages/github/__tests__/github.proxy.test.ts
  ● @actions/github › should only use default agent if one is not provided

    fetch is not set. Please pass a fetch implementation as new Octokit({ request: { fetch }}). Learn more at https://github.com/octokit/octokit.js/#fetch-missing

      at fetchWrapper (packages/github/node_modules/@octokit/request/dist-node/index.js:57:11)
      at request2 (packages/github/node_modules/@octokit/request/dist-node/index.js:180:14)
      at hook (packages/github/node_modules/@octokit/auth-token/dist-node/index.js:58:10)
      at packages/github/node_modules/before-after-hook/lib/register.js:25:15

That's not a difficult thing to fix, but the purpose of this PR is not to extend which versions of Node we support, only to recognize which versions users already depend on.

@joshmgross joshmgross merged commit bb2278e into main Nov 8, 2024
17 checks passed
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.

2 participants