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

fix: Prevent merging of poolOptions #5221

Merged
merged 4 commits into from
Feb 16, 2024
Merged

Conversation

penalosa
Copy link
Contributor

@penalosa penalosa commented Feb 16, 2024

Description

Arrays in poolOptions are merged with themselves, resulting in duplicating the entries. This PR fixes that, by ensuring poolOptions is not present in the config being merged. Bug repro at https://github.com/penalosa/vitest-custom-pool-repro (just clone and run ./test.sh to see the contents of the array option being duplicated),

See solidjs/vite-plugin-solid#101 for a similar fix

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • Discussed on Discord
  • Tests TBD
  • No lock file changes

Tests

  • Run the tests with pnpm test:ci.

Documentation

  • No new functionality, so no docs needed

Changesets

  • TBD

Copy link

netlify bot commented Feb 16, 2024

Deploy Preview for fastidious-cascaron-4ded94 canceled.

Name Link
🔨 Latest commit 3c17549
🔍 Latest deploy log https://app.netlify.com/sites/fastidious-cascaron-4ded94/deploys/65cf89206690db0008bb9e43

@penalosa penalosa changed the title Prevent merging of poolOptions fix: Prevent merging of poolOptions Feb 16, 2024
@sheremet-va
Copy link
Member

Can you add a test case in test/config?

@penalosa
Copy link
Contributor Author

Can you add a test case in test/config?

I added one in test/run, but I can add one in config as well

@sheremet-va
Copy link
Member

Can you add a test case in test/config?

I added one in test/run, but I can add one in config as well

I wrote the comment when there were no tests. I suggest adding a test somewhere here: https://github.com/vitest-dev/vitest/blob/main/test/config/test/resolution.test.ts

@penalosa
Copy link
Contributor Author

@sheremet-va I've added a test to that file now

@sheremet-va
Copy link
Member

Looks like this PR breaks other config options: https://github.com/vitest-dev/vitest/actions/runs/7932413357/job/21658846975?pr=5221

@sheremet-va sheremet-va merged commit bc5b2d0 into vitest-dev:main Feb 16, 2024
19 checks passed
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