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

Conversation

hereje
Copy link
Contributor

@hereje hereje commented Nov 30, 2023

Summary

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

related to #5695


For us to review and ship your PR efficiently, please perform the following steps:

  • Open a bug/issue before writing your code 🧑‍💻. This ensures we can discuss the changes and get feedback from everyone that should be involved. If you`re fixing a typo or something that`s on fire 🔥 (e.g. incident related), you can skip this step.
  • Read the contribution guidelines 📖. This ensures your code follows our style guide and
    passes our tests.
  • Update or add tests (if any source code was changed or added) 🧪
  • Update or add documentation (if features were changed or added) 📝
  • Make sure the status checks below are successful ✅

A picture of a cute animal (not mandatory, but encouraged)

@hereje hereje requested a review from a team as a code owner November 30, 2023 23:43
Copy link

github-actions bot commented Nov 30, 2023

📊 Benchmark results

Comparing with 51f1cf6

  • Dependency count: 1,359 (no change)
  • Package size: 313 MB (no change)
  • Number of ts-expect-error directives: 980 (no change)

Copy link

📊 Benchmark results

Comparing with 3268f29

  • Dependency count: 1,398 (no change)
  • Package size: 404 MB (no change)
  • Number of ts-expect-error directives: 0 (no change)

sarahetter
sarahetter previously approved these changes Dec 7, 2023
@sarahetter sarahetter added the automerge Add to Kodiak auto merge queue label Dec 13, 2023
@sarahetter sarahetter removed the automerge Add to Kodiak auto merge queue label Dec 14, 2023
@mrstork
Copy link
Contributor

mrstork commented May 9, 2024

For a while there looked to be several unrelated errors preventing this from getting merged, but most of those flaky tests look to have been resolved in main so I took a stab at merging the latest into this branch. Unfortunately though, the most recent run does look to have a genuine failure related to the code this function is testing, though it very well might be specific to windows + node 20.

FAIL  tests/integration/commands/dev/dev-miscellaneous.test.js > commands/dev-miscellaneous > should detect content changes in edge functions
AssertionError: expected 'There was an error with an Edge Funct…' to deeply equal 'Hello builder'

- Expected
+ Received

- Hello builder
+ There was an error with an Edge Function. Please check the terminal for more details.

When I have some time again, I will try to spin this up on a local windows instance to see if I can uncover why the edge function might be returning an error in this case.

@@ -789,7 +788,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)

@mrstork mrstork added the automerge Add to Kodiak auto merge queue label May 17, 2024
@kodiakhq kodiakhq bot merged commit 5c49780 into netlify:main May 17, 2024
46 checks passed
@mrstork mrstork mentioned this pull request May 17, 2024
5 tasks
@hereje hereje deleted the refactor/replace-got/dev-miscellaneous.test.js branch May 17, 2024 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Add to Kodiak auto merge queue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants