Cover branches in Tool.php and fix validation logic#41
Merged
Conversation
This commit addresses code coverage gaps in `src/Tool/Tool.php` for the following areas: - `initializeMetadata`: I added tests for `destructiveHint`, `idempotentHint`, and `openWorldHint` annotations. - `initialize`/`shutdown`: I added tests to verify these lifecycle methods are callable and log their execution. - `validateArguments`/`validateType`: I added tests for 'integer', 'object', and 'any' parameter types. During the process of adding tests for `validateArguments`, I identified and fixed the following bugs in `src/Tool/Tool.php`: - `validateArguments`: I changed `!isset(\$arguments[\$name])` to `!array_key_exists(\$name, \$arguments)` to correctly handle required arguments that are present with a `null` value. - `validateArguments`: I changed `if (isset(\$arguments[\$name]))` to `if (array_key_exists(\$name, \$arguments))` to ensure type validation occurs even for `null` values if the argument key is present. - `validateType`: I added an explicit check to allow `null` for the 'any' type and to disallow `null` by default for other basic types. All new and existing tests pass, and linters are clean for the `src` directory. PHPStan errors remain in a test fixture but do not impact the production code.
Codecov ReportAll modified and coverable lines are covered by tests ✅ 📢 Thoughts on this report? Let us know! |
This commit addresses feedback on the previous submission:
1. **Cover `\$value === null` branch in `Tool::validateType`**:
- I added a new test case (`testValidateTypeWithNullForNonAnyTypes`)
to `tests/Tool/ToolTest.php`.
- This test specifically verifies that `validateType` returns `false`
when a non-'any' type parameter (e.g., 'string', 'integer')
is given a `null` value.
2. **Resolve PHPStan errors in `tests/Tool/Fixture/ValidationTestTool.php`**:
- I removed the `#[Parameter]` attributes from the `doExecute` method
in `ValidationTestTool.php`.
- These attributes were causing PHPStan errors as they were placed
on a method instead of a parameter, and were not functionally
required for the existing tests (which use reflection to set
parameter metadata for `Tool::validateArguments` testing).
All PHPUnit tests, PHPCS, and PHPStan checks now pass.
🧪 Test Results SummaryPHPUnit Tests✅ All tests passed! Details: OK (232 tests, 939 assertions)
Code Quality
📁 Detailed Reports
This comment will update automatically when you push new commits. Code Coverage
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This commit addresses code coverage gaps in
src/Tool/Tool.phpfor the following areas:initializeMetadata: I added tests fordestructiveHint,idempotentHint, andopenWorldHintannotations.initialize/shutdown: I added tests to verify these lifecycle methods are callable and log their execution.validateArguments/validateType: I added tests for 'integer', 'object', and 'any' parameter types.During the process of adding tests for
validateArguments, I identified and fixed the following bugs insrc/Tool/Tool.php:validateArguments: I changed!isset(\$arguments[\$name])to!array_key_exists(\$name, \$arguments)to correctly handle required arguments that are present with anullvalue.validateArguments: I changedif (isset(\$arguments[\$name]))toif (array_key_exists(\$name, \$arguments))to ensure type validation occurs even fornullvalues if the argument key is present.validateType: I added an explicit check to allownullfor the 'any' type and to disallownullby default for other basic types.All new and existing tests pass, and linters are clean for the
srcdirectory. PHPStan errors remain in a test fixture but do not impact the production code.