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: tests/integration/20.command.functions.test.cjs #5889

Conversation

hereje
Copy link
Contributor

@hereje hereje commented Jul 22, 2023

refactor test file to ESM

Summary

  • convert to ESM
  • replace ava with vitest
  • replace got with node-fetch

Related to: #5698


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)

refactor test file to ESM

- convert to ESM
- replace ava with vitest
- replace got with node-fetch

Related to: netlify#5698
@github-actions
Copy link

github-actions bot commented Jul 22, 2023

📊 Benchmark results

Comparing with 8c2b60c

  • Dependency count: 1,303 (no change)
  • Package size: 271 MB ⬆️ 0.00% increase vs. 8c2b60c

@hereje hereje marked this pull request as ready for review July 25, 2023 14:49
@hereje hereje requested a review from a team as a code owner July 25, 2023 14:49
lukasholzer
lukasholzer previously approved these changes Jul 26, 2023
Copy link
Collaborator

@lukasholzer lukasholzer left a comment

Choose a reason for hiding this comment

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

AWESOME JOB! 🐳

.buildAsync()

await withFunctionsServer({ builder }, async () => {
const response = await fetch(`http://localhost:9999/.netlify/functions/ping`)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for using fetch and getting rid of got!

await builder
.withNetlifyToml({
config: {
build: { environment: { VITEST_TEST: 'FROM_CONFIG_FILE' } },
Copy link
Collaborator

Choose a reason for hiding this comment

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

NIT: As mentioned in the other PR I would not rename the variable to something that could be set by vitest (to avoid having it set from the testrunner)

I would rename it to something like MY_CONFIG

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

Copy link
Contributor Author

@hereje hereje left a comment

Choose a reason for hiding this comment

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

Done

Copy link
Collaborator

@lukasholzer lukasholzer left a comment

Choose a reason for hiding this comment

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

Awesome!

@lukasholzer lukasholzer merged commit bcb1e7b into netlify:main Jul 27, 2023
@hereje hereje deleted the refactor_test_esm/tests/integration/20.command.functions.test.cjs branch July 27, 2023 13:43
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