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

refactor: replace got with node-fetch on dev-miscellaneous.test.js #6235

Merged
Merged
Changes from 32 commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
4248090
refactor: replace got with node-fetch on dev-miscellaneous.test.js
hereje Nov 30, 2023
dd3a820
Merge branch 'main' into refactor/replace-got/dev-miscellaneous.test.js
hereje Nov 30, 2023
ff37898
Merge branch 'main' into refactor/replace-got/dev-miscellaneous.test.js
hereje Dec 5, 2023
12a17de
Merge branch 'main' into refactor/replace-got/dev-miscellaneous.test.js
hereje Dec 7, 2023
c3b6577
Merge branch 'main' into refactor/replace-got/dev-miscellaneous.test.js
hereje Dec 8, 2023
7f7f51b
Merge branch 'main' into refactor/replace-got/dev-miscellaneous.test.js
hereje Dec 9, 2023
8460a4f
Merge branch 'main' into refactor/replace-got/dev-miscellaneous.test.js
hereje Dec 12, 2023
cd52475
Merge branch 'netlify:main' into refactor/replace-got/dev-miscellaneo…
hereje Dec 13, 2023
54c995d
Merge branch 'main' into refactor/replace-got/dev-miscellaneous.test.js
sarahetter Dec 13, 2023
ec7042e
Merge branch 'netlify:main' into refactor/replace-got/dev-miscellaneo…
hereje Dec 13, 2023
3f3b1a8
Merge branch 'main' into refactor/replace-got/dev-miscellaneous.test.js
sarahetter Dec 14, 2023
d5d3ba7
Merge branch 'main' into refactor/replace-got/dev-miscellaneous.test.js
hereje Dec 18, 2023
16c44f1
Merge branch 'main' into refactor/replace-got/dev-miscellaneous.test.js
hereje Dec 20, 2023
aed1570
Merge branch 'netlify:main' into refactor/replace-got/dev-miscellaneo…
hereje Dec 20, 2023
88b429c
Merge branch 'main' into refactor/replace-got/dev-miscellaneous.test.js
hereje Dec 26, 2023
3c740fe
Merge branch 'main' into refactor/replace-got/dev-miscellaneous.test.js
hereje Jan 2, 2024
48de871
Merge branch 'netlify:main' into refactor/replace-got/dev-miscellaneo…
hereje Jan 11, 2024
6df4f23
Merge branch 'main' into refactor/replace-got/dev-miscellaneous.test.js
hereje Jan 11, 2024
112b208
Merge branch 'netlify:main' into refactor/replace-got/dev-miscellaneo…
hereje Jan 14, 2024
34263b5
Merge branch 'main' into refactor/replace-got/dev-miscellaneous.test.js
hereje Jan 23, 2024
1998f17
Merge branch 'main' into refactor/replace-got/dev-miscellaneous.test.js
hereje Feb 20, 2024
3af26ca
Merge branch 'main' into refactor/replace-got/dev-miscellaneous.test.js
hereje Feb 21, 2024
cc7e0d3
Merge branch 'main' into refactor/replace-got/dev-miscellaneous.test.js
hereje Feb 21, 2024
b2dae41
Merge branch 'main' into refactor/replace-got/dev-miscellaneous.test.js
hereje Feb 23, 2024
5777c9e
Merge branch 'main' into refactor/replace-got/dev-miscellaneous.test.js
hereje Feb 28, 2024
2d27359
Merge branch 'main' into refactor/replace-got/dev-miscellaneous.test.js
hereje Mar 1, 2024
7a4a77b
Merge branch 'main' into refactor/replace-got/dev-miscellaneous.test.js
hereje Mar 4, 2024
f27672a
Merge branch 'main' into refactor/replace-got/dev-miscellaneous.test.js
hereje Mar 6, 2024
d418f20
Merge branch 'main' into refactor/replace-got/dev-miscellaneous.test.js
sarahetter Apr 22, 2024
c5ef5b2
Merge branch 'netlify:main' into refactor/replace-got/dev-miscellaneo…
hereje Apr 23, 2024
ba00765
Merge branch 'main' into refactor/replace-got/dev-miscellaneous.test.js
sarahetter May 2, 2024
9e0cf9c
Merge branch 'main' into refactor/replace-got/dev-miscellaneous.test.js
hereje May 6, 2024
c18d231
Merge branch 'main' into refactor/replace-got/dev-miscellaneous.test.js
mrstork May 9, 2024
5a2f690
Merge branch 'main' into refactor/replace-got/dev-miscellaneous.test.js
mrstork May 17, 2024
b610220
Merge branch 'main' into refactor/replace-got/dev-miscellaneous.test.js
kodiakhq[bot] May 17, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 2 additions & 3 deletions tests/integration/commands/dev/dev-miscellaneous.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import fetch from 'node-fetch'
import { describe, test } from 'vitest'

import { withDevServer } from '../../utils/dev-server.ts'
import got from '../../utils/got.js'
import { withMockApi } from '../../utils/mock-api.js'
import { pause } from '../../utils/pause.js'
import { withSiteBuilder } from '../../utils/site-builder.ts'
Expand Down Expand Up @@ -774,7 +773,7 @@ describe.concurrent('commands/dev-miscellaneous', () => {
await builder.build()

await withDevServer({ cwd: builder.directory }, async ({ port }) => {
const helloWorldMessage = await got(`http://localhost:${port}/hello`).then((res) => res.body)
const helloWorldMessage = await fetch(`http://localhost:${port}/hello`).then((res) => res.text())

await builder
.withEdgeFunction({
Expand All @@ -786,7 +785,7 @@ describe.concurrent('commands/dev-miscellaneous', () => {
const DETECT_FILE_CHANGE_DELAY = 500
await pause(DETECT_FILE_CHANGE_DELAY)
Copy link
Contributor

Choose a reason for hiding this comment

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

I played around with removing this delay and was able to uncover something potentially useful, but did not ultimately uncover the root of the problem. With no pause, we will see different results:

mac + node v20.12.2 + got -> 'Hello world'
windows + node v20.12.2 + got -> 'Hello world'

mac + node v20.12.2 + fetch -> 'Hello world'
windows + node v20.12.2 + fetch -> ECONNREFUSED

I also found this potentially related node 18 issue node-fetch/node-fetch#1624. Even though the test is failing on a different version of node, I tried changing the url in the test from localhost to 127.0.0.1 but saw no change in behaviour.

It doesn't actually solve the underlying issue, but given the above, increasing this delay up from 500 should get the tests to pass.

Copy link
Contributor

@mrstork mrstork May 13, 2024

Choose a reason for hiding this comment

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

To be clear, the changes in this PR do not introduce an issue (as they are just modifying a test), they simply bring to light an issue that exists somewhere in our codebase. Some interaction between some or all of the following components: edge proxy, fetch, builder (on windows)


const helloBuilderMessage = await got(`http://localhost:${port}/hello`).then((res) => res.body)
const helloBuilderMessage = await fetch(`http://localhost:${port}/hello`, {}).then((res) => res.text())

t.expect(helloWorldMessage).toEqual('Hello world')
t.expect(helloBuilderMessage).toEqual('Hello builder')
Expand Down