Skip to content

Comments

fix: Prevent merging of poolOptions#5221

Merged
sheremet-va merged 4 commits intovitest-dev:mainfrom
penalosa:patch-1
Feb 16, 2024
Merged

fix: Prevent merging of poolOptions#5221
sheremet-va merged 4 commits intovitest-dev:mainfrom
penalosa:patch-1

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

@netlify
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

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