Skip to content

Extend Node version test coverage#1843

Merged
joshmgross merged 4 commits intomainfrom
joshmgross/extend-node-test-coverage
Nov 8, 2024
Merged

Extend Node version test coverage#1843
joshmgross merged 4 commits intomainfrom
joshmgross/extend-node-test-coverage

Conversation

@joshmgross
Copy link
Copy Markdown
Contributor

@joshmgross joshmgross commented Oct 4, 2024

#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
Copy Markdown
Contributor Author

joshmgross commented Oct 4, 2024

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 joshmgross mentioned this pull request Oct 4, 2024
Merged
@joshmgross
Copy link
Copy Markdown
Contributor Author

joshmgross commented Oct 7, 2024

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@luketomlinson luketomlinson luketomlinson 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.

2 participants

@joshmgross @luketomlinson