chore: add test for boolean behavior and rename embeddingsValidation #759
Conversation
| * Preprocessor for boolean values that handles string "false"/"0" correctly. | ||
| * Zod's coerce.boolean() treats any non-empty string as true, which is not what we want. | ||
| */ | ||
| export function parseBoolean(val: unknown): unknown { |
There was a problem hiding this comment.
This is actually largely irrelevant for CLI-based parsing because yargs-parser takes care of this for us but it's generally better to have this for other types of input.
There was a problem hiding this comment.
I could remove this to be honest, it's more relevant for headers case but it still doesn't hurt
There was a problem hiding this comment.
Pull request overview
This PR renames the embeddings validation configuration from disableEmbeddingsValidation to embeddingsValidation with inverted boolean semantics, and adds comprehensive boolean parsing support for CLI arguments.
Key Changes:
- Renamed
disableEmbeddingsValidationtoembeddingsValidation(flipped from "disable" to "enable" semantics) - Implemented
parseBooleanhelper to correctly handle string "false"/"0" values in CLI arguments - Added extensive test coverage for boolean CLI argument parsing behavior
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/common/config/userConfig.ts | Renamed field from disableEmbeddingsValidation to embeddingsValidation, inverted default from false to true, and added parseBoolean preprocessing for boolean fields |
| src/common/config/configUtils.ts | Added parseBoolean function to correctly parse boolean values from strings, and improved commaSeparatedToArray implementation |
| src/common/config/argsParserOptions.ts | Updated boolean field name in OPTIONS configuration |
| src/common/search/vectorSearchEmbeddingsManager.ts | Updated logic to use embeddingsValidation with inverted boolean check and removed redundant validation check in embed method |
| tests/unit/common/config.test.ts | Updated test expectations to use new field name and added comprehensive tests for boolean CLI argument parsing |
| tests/unit/common/search/vectorSearchEmbeddingsManager.test.ts | Updated test fixtures to use new field name with inverted boolean values |
| server.json | Updated configuration metadata with renamed field and reordered entries alphabetically |
| scripts/generateArguments.ts | Removed assertion that required array descriptions to start with "An array of" |
| README.md | Updated documentation to reflect renamed configuration field |
| cli: ["--readOnly", "FALSE"], | ||
| // This is yargs-parser default | ||
| expected: { readOnly: true }, |
There was a problem hiding this comment.
The test expects uppercase 'FALSE' to result in true, but the parseBoolean function converts to lowercase and checks for 'false', which would return false. This test case doesn't match the actual implementation behavior.
There was a problem hiding this comment.
you're understandably confused but this isn't actually hitting the string case, yargs-parser gives a boolean-based situation
Pull Request Test Coverage Report for Build 19676499266Details
💛 - Coveralls |
This is actually largely irrelevant for CLI-based parsing because yargs-parser takes care of this for us but it's generally better to have this for other types of input.
So...
Two steps:
chore: add tests and pre-processor for other cases
chore: disableEmbeddingsValidation -> embeddingsValidation