Test against Node 20, update tests for compatibility #7636
Conversation
✅ Deploy Preview for apollo-server-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit c7e0570:
|
|
Stuck on the last couple tests where the connection shouldn't be closing but it is (for Node 20). This PR seems to be related, but my attempts to apply the information provided haven't been fruitful so far. I've tried adding the |
| @@ -382,7 +382,7 @@ export function defineIntegrationTestSuiteHttpServerTests( | |||
| return req.then((res) => { | |||
| expect(res.status).toEqual(400); | |||
| expect( | |||
| ['Unexpected token f', 'Bad Request', 'Invalid JSON'].some( | |||
| ['in JSON at position 1', 'Bad Request', 'Invalid JSON'].some( | |||
There was a problem hiding this comment.
Error message changed in Node 20, this substring is compatible across all of our supported versions.
| @@ -193,7 +193,7 @@ it('supporting doubly-encoded variables example from migration guide', async () | |||
| query: 'query Hello($s: String!){hello(s: $s)}', | |||
| variables: '{malformed JSON}', | |||
| }) | |||
| .expect(400, 'Unexpected token m in JSON at position 1'); | |||
| .expect(400, /in JSON at position 1/); | |||
There was a problem hiding this comment.
Error message changed in Node 20, this substring is compatible across all of our supported versions.
| @@ -113,7 +112,7 @@ Object.keys(schemes).forEach((schemeName) => { | |||
| const err = await a.failure( | |||
| request(`${schemeName}://localhost:${p}`).agent(scheme.agent()), | |||
| ); | |||
| expect(err.message).toMatch(/ECONNREFUSED/); | |||
| expect(err.code).toMatch(/ECONNREFUSED/); | |||
There was a problem hiding this comment.
.message is undefined in Node 20 but .code is defined across all of our supported versions.
| expect(err.code).toMatch(/ECONNREFUSED/); | ||
|
|
||
| const isNode20 = !!process.version.match(/^v20\./); | ||
| expect(closed).toBe(isNode20 ? 1 : 0); |
There was a problem hiding this comment.
(currently) unexplained change in behavior in http(s).Server, but maybe this isn't actually important for us to validate/enforce. nodejs/node#46333 seems to be the only relevant change listed in the Node 20 changelog but I haven't been able to apply the information there in a useful way that "corrects" this test. That PR also only seems to apply to when expected headers don't exist, which isn't the case here.
There was a problem hiding this comment.
For future reference, @glasser helped me pin this to changes made in v19 and v20.3
(http) nodejs/node#43522
(https equiv) nodejs/node#48383
| @@ -301,12 +302,21 @@ Object.keys(schemes).forEach((schemeName) => { | |||
|
|
|||
| if (schemeName === 'http') { | |||
| it('with in-flights finishing before grace period ends', async () => { | |||
There was a problem hiding this comment.
This test was causing an open handle / AggregateError that I was unfruitful in tracking down. Seems to have been related to spawning a process (maybe the server in the process wasn't being killed?). In any case, I think this change simplifies the test a bit by eliminating the need to spawn a process.
trevor-scheer
force-pushed
the
trevor/test-node-20
branch
from
f8535b9
to
c7e0570
Compare
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @apollo/server-integration-testsuite@4.8.1 ### Patch Changes - [#7636](#7636) [`42fc65cb2`](42fc65c) Thanks [@trevor-scheer](https://github.com/trevor-scheer)! - Update test suite for compatibility with Node v20 - Updated dependencies \[[`42fc65cb2`](42fc65c)]: - @apollo/server@4.8.1 ## @apollo/server@4.8.1 ### Patch Changes - [#7636](#7636) [`42fc65cb2`](42fc65c) Thanks [@trevor-scheer](https://github.com/trevor-scheer)! - Update test suite for compatibility with Node v20 Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Test against Node 20 and update tests as needed.
This unblocks our integrations from testing against Node 20 as well since the testsuite is part of this changeset / has Node 20 test failures.