Added assertions from file. #37
Conversation
WalkthroughThe pull request introduces new configuration options and method definitions for handling fixture files in the API server context. The Changes
Sequence Diagram(s)sequenceDiagram
participant T as Test
participant A as ApiServerContext
participant S as API Server
participant L as Logger
T->>A: Invoke "reset API" step
A->>S: Send DELETE request to /responses
A->>S: Send DELETE request to /requests
A->>L: Log debug message "API Reset"
A-->>T: Return reset confirmation
sequenceDiagram
participant T as Test
participant A as ApiServerContext
participant FS as Fixture Reader
participant S as API Server
participant L as Logger
T->>A: Request file response (e.g., "test_data.json")
A->>FS: Search fixture paths for file
FS-->>A: Return file content and extension
A->>L: Log file response details
A->>S: Set response with content & detected content type
A-->>T: Return file response setup confirmation
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## main #37 +/- ##
==========================================
+ Coverage 70.47% 74.87% +4.40%
==========================================
Files 3 3
Lines 359 402 +43
==========================================
+ Hits 253 301 +48
+ Misses 106 101 -5 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
README.md (1)
183-183: Markdown Code Block StyleA static analysis hint noted that the code block style at this location might differ from expected guidelines. Please verify that using fenced code blocks (instead of indented ones) aligns with your project’s markdown style conventions.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
183-183: Code block style
Expected: indented; Actual: fenced(MD046, code-block-style)
tests/behat/bootstrap/FeatureContext.php (1)
117-127: Potential duplication with existing method.This method appears to duplicate functionality that already exists in
responseHeaderContains()at line 77. The main differences are:
- This method uses
str_containswithout a backslash- This method doesn't use
strtolower, making it case-sensitiveConsider either:
- Consolidating these methods to avoid duplication
- Updating documentation to clarify the difference in case-sensitivity behavior
- Making both methods consistent in their case-sensitivity approach
- public function theResponseHeaderShouldContain(string $name, string $value): void { - $actual = (string) $this->getSession()->getResponseHeader($name); - $message = sprintf('The header "%s" does not contain the value "%s", but has a value of "%s"', $name, $value, $actual); - - if (!str_contains($actual, $value)) { - throw new \Exception($message); - } - }src/DrevOps/BehatPhpServer/ApiServerContext.php (1)
200-271: Improve error handling coverage in the file response method.The method for handling file responses is well-implemented, but the static analysis tool indicates that several error handling paths aren't covered by tests:
- Exception when file is not found in any fixture path (lines 240-244)
- Exception when file cannot be read (line 249)
- Default content type for unknown file extensions (line 259)
Consider adding tests to cover these error scenarios to ensure robust error handling. For example:
- Test with a non-existent file
- Test with a file that has unrecognized extension
- Simulate a file read error (might require mocking)
Additionally, the match expression for content types is clean, but I notice the default extension doesn't get test coverage:
$content_type = match (strtolower($extension)) { 'json' => 'application/json', 'xml' => 'application/xml', 'html', 'htm' => 'text/html', 'txt' => 'text/plain', default => 'application/octet-stream', };🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 240-244: src/DrevOps/BehatPhpServer/ApiServerContext.php#L240-L244
Added lines #L240 - L244 were not covered by tests
[warning] 249-249: src/DrevOps/BehatPhpServer/ApiServerContext.php#L249
Added line #L249 was not covered by tests
[warning] 259-259: src/DrevOps/BehatPhpServer/ApiServerContext.php#L259
Added line #L259 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
README.md(2 hunks)behat.yml(1 hunks)src/DrevOps/BehatPhpServer/ApiServerContext.php(3 hunks)tests/behat/bootstrap/FeatureContext.php(2 hunks)tests/behat/features/apiserver.feature(4 hunks)tests/behat/fixtures/test_content.xml(1 hunks)tests/behat/fixtures/test_data.json(1 hunks)tests/behat/fixtures2/secondary_data.txt(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
README.md
183-183: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
🪛 GitHub Check: codecov/patch
src/DrevOps/BehatPhpServer/ApiServerContext.php
[warning] 85-86: src/DrevOps/BehatPhpServer/ApiServerContext.php#L85-L86
Added lines #L85 - L86 were not covered by tests
[warning] 88-88: src/DrevOps/BehatPhpServer/ApiServerContext.php#L88
Added line #L88 was not covered by tests
[warning] 90-90: src/DrevOps/BehatPhpServer/ApiServerContext.php#L90
Added line #L90 was not covered by tests
[warning] 93-93: src/DrevOps/BehatPhpServer/ApiServerContext.php#L93
Added line #L93 was not covered by tests
[warning] 240-244: src/DrevOps/BehatPhpServer/ApiServerContext.php#L240-L244
Added lines #L240 - L244 were not covered by tests
[warning] 249-249: src/DrevOps/BehatPhpServer/ApiServerContext.php#L249
Added line #L249 was not covered by tests
[warning] 259-259: src/DrevOps/BehatPhpServer/ApiServerContext.php#L259
Added line #L259 was not covered by tests
🔇 Additional comments (21)
tests/behat/fixtures2/secondary_data.txt (1)
1-4: Fixture File Content is Clear & InformativeThe file clearly indicates its purpose and provides concise text to support testing of multi-path fixture functionality. No changes needed.
tests/behat/fixtures/test_data.json (1)
1-29: JSON Fixture Data is Well-StructuredThe JSON file is properly formatted with an array of product items and an accompanying metadata object. All key fields are present with appropriate data types, ensuring the file can support file-based responses as intended.
behat.yml (1)
22-24: New Fixture Paths Configuration is Correctly IntegratedThe addition of the
fixtures_pathskey under theApiServerContextconfiguration correctly specifies multiple directories. Please double-check that the paths ('%paths.base%/tests/behat/fixtures'and'%paths.base%/tests/behat/fixtures2') accurately reflect the repository structure.tests/behat/fixtures/test_content.xml (1)
1-27: XML Fixture File is Well-FormedThe XML content is clear and adheres to standard XML syntax. The catalog structure for books is precise and provides appropriate details for testing file-based API responses.
README.md (2)
74-77: Documentation Update: Fixture PathsThe README now documents the
fixtures_pathsconfiguration for theApiServerContextclearly, listing both fixture directories. This update will help users correctly set up and locate fixture files.
148-157: New Step Definitions Enhance API Response TestingThe newly added step definitions for resetting the API server and queuing file responses (with both automatic content type detection and custom response codes) are well integrated. The documentation clearly explains their usage, aligning with the updated API server functionality.
tests/behat/bootstrap/FeatureContext.php (4)
58-58: URL path updated to match the correct file name.The path has been correctly updated from
/testpage.htmlto/test_page.htmlto match the actual file name in the fixtures directory.
98-103: LGTM: Added convenient method for GET requests.This helper method simplifies the test steps by providing a dedicated method for GET requests, making the feature files more readable.
105-115: LGTM: Added method for exact header value matching.This method provides a strict equality check for header values, which complements the existing containing check methods.
129-144: LGTM: Added HTML response validation.This method properly checks both the Content-Type header and the response content to ensure it's HTML, providing a robust validation mechanism.
tests/behat/features/apiserver.feature (8)
1-6: LGTM: Enhanced feature description and metadata.The added description clearly explains the purpose of these tests, improving documentation and readability.
13-14: LGTM: Added reset step to ensure clean test state.Adding the reset step ensures that each test starts with a clean server state, preventing test interference.
20-40: LGTM: Added dedicated scenario for testing reset functionality.This scenario properly validates that the API server reset functionality correctly clears both queued responses and received requests.
188-198: LGTM: Added JSON file response validation.This scenario properly tests that the API server can respond with JSON file content and correctly sets the appropriate Content-Type header.
199-209: LGTM: Added XML file response validation.This scenario correctly tests that the API server can respond with XML file content with a custom status code and sets the appropriate Content-Type header.
210-218: LGTM: Added HTML file response validation.This scenario properly tests that the API server can respond with HTML file content and correctly validates both the Content-Type header and the HTML content.
219-227: LGTM: Added test for primary fixtures path.This scenario verifies that files can be loaded from the primary fixtures path, an important functionality for test organization.
228-235: LGTM: Added test for secondary fixtures path.This scenario verifies that files can be loaded from the secondary fixtures path, validating the multi-path fixture loading capability.
src/DrevOps/BehatPhpServer/ApiServerContext.php (3)
52-58: LGTM: Added property for storing fixture paths.The property is properly typed and documented, providing clear understanding of its purpose.
76-79: LGTM: Updated constructor parameter for fixture paths.The parameter is well-documented and properly typed to allow for different input formats.
115-125: LGTM: Added API server reset functionality.This method properly implements the reset functionality by sending DELETE requests to clear both responses and requests from the API server.
| // Set fixtures paths, with fallback to the default location. | ||
| if (empty($fixtures_paths)) { | ||
| $this->fixturesPaths[] = dirname($this->webroot) . '/tests/behat/fixtures'; | ||
| } | ||
| elseif (is_array($fixtures_paths)) { | ||
| // Handle both string and array. | ||
| $this->fixturesPaths = array_map('strval', $fixtures_paths); | ||
| } | ||
| else { | ||
| $this->fixturesPaths[] = (string) $fixtures_paths; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Address uncovered code paths in fixture path handling.
The static analysis tool indicates that several code paths for handling fixture paths aren't covered by tests. Consider adding tests for these scenarios:
- Default path when no fixtures paths are provided
- Handling array input for fixtures paths
- Handling string input for fixtures paths
🏁 Script executed:
#!/bin/bash
# Check test coverage for fixture paths initialization logic
# Search for tests that explicitly test different fixture path configurations
echo "Searching for tests covering fixture paths initialization..."
rg -A 10 "fixtures_paths" tests/
# Check if there are any tests instantiating ApiServerContext with different parameters
echo "Searching for tests instantiating ApiServerContext with various parameters..."
rg -A 5 "new ApiServerContext" tests/Length of output: 368
Missing Test Coverage for Fixture Path Initialization
Our investigation did not reveal any tests covering the various fixture path logic in src/DrevOps/BehatPhpServer/ApiServerContext.php (lines 84–94). This confirms the static analysis warning. Please add tests that cover the following scenarios:
- When no fixtures paths are provided (default fallback path).
- When an array is provided (ensuring proper conversion using
array_map('strval', ...)). - When a string is provided (casting to string correctly).
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 85-86: src/DrevOps/BehatPhpServer/ApiServerContext.php#L85-L86
Added lines #L85 - L86 were not covered by tests
[warning] 88-88: src/DrevOps/BehatPhpServer/ApiServerContext.php#L88
Added line #L88 was not covered by tests
[warning] 90-90: src/DrevOps/BehatPhpServer/ApiServerContext.php#L90
Added line #L90 was not covered by tests
[warning] 93-93: src/DrevOps/BehatPhpServer/ApiServerContext.php#L93
Added line #L93 was not covered by tests
Signed-off-by: Alex Skrypnyk <[email protected]>
AlexSkrypnyk
force-pushed
the
feature/file-assertions
branch
from
2ff991d to
afff2b3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
README.md (1)
177-188: Code block formatting inconsistency.The gherkin example at lines 183-188 uses fenced code block style (`````) while the markdown linter expects indented code blocks.
Consider updating to use indented code block style to be consistent with the linter requirements.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
183-183: Code block style
Expected: indented; Actual: fenced(MD046, code-block-style)
tests/behat/bootstrap/FeatureContext.php (1)
117-127: Method duplication concern.This method
theResponseHeaderShouldContainappears to be very similar to the existingresponseHeaderContainsmethod at lines 77-84.Consider refactoring to eliminate this duplication by either:
- Using a single method for both step definitions
- Having one method call the other to avoid code duplication
-public function theResponseHeaderShouldContain(string $name, string $value): void { - $actual = (string) $this->getSession()->getResponseHeader($name); - $message = sprintf('The header "%s" does not contain the value "%s", but has a value of "%s"', $name, $value, $actual); - - if (!str_contains($actual, $value)) { - throw new \Exception($message); - } +public function theResponseHeaderShouldContain(string $name, string $value): void { + $this->responseHeaderContains($name, $value); }src/DrevOps/BehatPhpServer/ApiServerContext.php (4)
76-79: Consider adding proper type hint for the paths parameter.The
$pathsparameter is documented in PHPDoc but lacks an explicit type hint in the method signature. While this is understandable since it accepts both string and array, consider using PHP 8.0's union types (array|string|null) if your minimum PHP version allows it.- @param string[]|string|null $paths + @param array<string>|string|null $paths- public function __construct(?string $webroot = NULL, string $host = '127.0.0.1', int $port = 8888, string $protocol = 'http', bool $debug = FALSE, ?int $connection_timeout = NULL, ?int $retry_delay = NULL, $paths = NULL) { + public function __construct(?string $webroot = NULL, string $host = '127.0.0.1', int $port = 8888, string $protocol = 'http', bool $debug = FALSE, ?int $connection_timeout = NULL, ?int $retry_delay = NULL, array|string|null $paths = NULL) {
115-125: Consider checking response status codes in resetApi method.The
resetApi()method correctly sends DELETE requests to clear responses and requests, but it doesn't check if these operations were successful. Consider adding status code checks similar to other methods in this class.public function resetApi(): void { - $this->client->request('DELETE', '/admin/responses'); - $this->client->request('DELETE', '/admin/requests'); + $response1 = $this->client->request('DELETE', '/admin/responses'); + if ($response1->getStatusCode() !== 200) { + throw new \RuntimeException('Failed to reset API responses.'); + } + + $response2 = $this->client->request('DELETE', '/admin/requests'); + if ($response2->getStatusCode() !== 200) { + throw new \RuntimeException('Failed to reset API requests.'); + } $this->debug('API server responses and requests have been reset.'); }
228-237: Use DIRECTORY_SEPARATOR for cross-platform compatibility.The method concatenates paths with a hardcoded '/' character, which may cause issues on different operating systems. Consider using PHP's
DIRECTORY_SEPARATORconstant or consider using a proper path joining function.foreach ($this->fixturesPaths as $fixtures_path) { - $path = $fixtures_path . '/' . $file_path; + $path = $fixtures_path . DIRECTORY_SEPARATOR . $file_path; $error_paths[] = $fixtures_path; if (file_exists($path)) { $absolute_path = $path; $file_found = TRUE; break; } }
253-260: Consider supporting additional common MIME types.The content type detection based on file extension is a good approach, but you might want to support additional common file types like CSV, PDF, images (JPEG, PNG, GIF), etc.
$content_type = match (strtolower($extension)) { 'json' => 'application/json', 'xml' => 'application/xml', 'html', 'htm' => 'text/html', 'txt' => 'text/plain', + 'csv' => 'text/csv', + 'pdf' => 'application/pdf', + 'jpg', 'jpeg' => 'image/jpeg', + 'png' => 'image/png', + 'gif' => 'image/gif', default => 'application/octet-stream', };Also, add tests to cover the default case (line 259) which is currently not covered according to static analysis.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 259-259: src/DrevOps/BehatPhpServer/ApiServerContext.php#L259
Added line #L259 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
README.md(2 hunks)behat.yml(1 hunks)src/DrevOps/BehatPhpServer/ApiServerContext.php(3 hunks)tests/behat/bootstrap/FeatureContext.php(2 hunks)tests/behat/features/apiserver.feature(4 hunks)tests/behat/fixtures/test_content.xml(1 hunks)tests/behat/fixtures/test_data.json(1 hunks)tests/behat/fixtures2/secondary_data.txt(1 hunks)tests/phpunit/Unit/ApiServerContextTest.php(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/behat/fixtures2/secondary_data.txt
- tests/behat/fixtures/test_data.json
- tests/behat/fixtures/test_content.xml
- behat.yml
🧰 Additional context used
🧬 Code Definitions (1)
tests/phpunit/Unit/ApiServerContextTest.php (2)
src/DrevOps/BehatPhpServer/ApiServerContext.php (1)
ApiServerContext(20-345)tests/phpunit/Traits/ReflectionTrait.php (1)
getProtectedValue(96-102)
🪛 markdownlint-cli2 (0.17.2)
README.md
183-183: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
🪛 GitHub Check: codecov/patch
src/DrevOps/BehatPhpServer/ApiServerContext.php
[warning] 240-244: src/DrevOps/BehatPhpServer/ApiServerContext.php#L240-L244
Added lines #L240 - L244 were not covered by tests
[warning] 249-249: src/DrevOps/BehatPhpServer/ApiServerContext.php#L249
Added line #L249 was not covered by tests
[warning] 259-259: src/DrevOps/BehatPhpServer/ApiServerContext.php#L259
Added line #L259 was not covered by tests
🔇 Additional comments (21)
README.md (3)
74-76: Configuration enhancement: Added paths configuration looks good.The new
pathsconfiguration for ApiServerContext allows specifying multiple fixture file locations, which enhances flexibility and organization.
149-156: Good addition of reset and file response documentation.These new step definitions enable important functionality for test reliability and simplify the setup of file-based responses.
162-176: Clear documentation of file response functionality.The documentation clearly explains the automatic content type detection based on file extensions and how the context searches through fixture paths, which will be helpful for users.
tests/phpunit/Unit/ApiServerContextTest.php (3)
9-18: Good use of ReflectionTrait for testing protected properties.Adding the ReflectionTrait allows for testing the internal state of the ApiServerContext, which is a good testing practice.
288-323: Well-structured test for fixture paths.The test method creates a temporary directory, instantiates the context with different path configurations, and verifies the results. It also properly cleans up after itself.
325-358: Comprehensive test data provider covers edge cases.The data provider includes important test cases:
- Default behavior with null paths
- Single string path
- Array of paths
- Empty array (falls back to default)
- Non-string scalar conversion
This ensures the path handling is robust under various input conditions.
tests/behat/bootstrap/FeatureContext.php (4)
58-58: Updated path to match actual filename.Corrected the path from
/testpage.htmlto/test_page.htmlwhich ensures the test can find the correct file.
98-104: Good addition of simplified GET request method.This helper method simplifies the Gherkin steps by providing a shorthand for GET requests.
105-115: Added precise header assertion method.The new method checks for exact header value matches, complementing the existing method that checks if headers contain values.
129-144: Thorough HTML response validation.The method properly checks both the Content-Type header and examines content for HTML tags, providing a more reliable verification than checking just the header.
tests/behat/features/apiserver.feature (7)
1-6: Improved feature description.The feature description now clearly explains the purpose and user story, making the tests more understandable.
13-14: Good practice: Added reset step.Adding the API server reset ensures a clean state for the test, which improves test reliability.
20-40: Good test for reset functionality.This scenario properly tests that the reset functionality clears both queued responses and request history.
188-197: Comprehensive test for JSON file responses.This scenario verifies both the content and the correct content type header for JSON file responses.
199-208: Good test for XML file responses with custom status code.The scenario verifies that both the content and the status code are correctly set when responding with an XML file.
210-217: Thorough test for HTML file responses.This scenario uses the new
theResponseShouldBeHtmlmethod to verify HTML responses properly.
219-235: Good tests for multiple fixture paths.These scenarios verify that files can be found in both primary and secondary fixture paths, which is an important part of the new functionality.
src/DrevOps/BehatPhpServer/ApiServerContext.php (4)
52-57: Good addition of the fixtures paths property!The new protected property
$fixturesPathswith proper PHPDoc is well-defined and appropriately initialized as an empty array.
84-94: Add tests for fixture paths initialization scenarios.The fixture path initialization logic handles multiple scenarios (null, array, string), but according to static analysis, these branches may not be well-covered by tests.
This confirms the static analysis warning from the previous review. Please ensure you add tests for:
- Default path when no fixtures paths are provided
- Handling array input for fixtures paths
- Handling string input for fixtures paths
247-250: Add tests for file read failure error path.The static analysis indicates this error handling code is not covered by tests. Consider adding a test case that simulates a file read failure.
This is a critical error path that should be tested to ensure proper error handling when file operations fail.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 249-249: src/DrevOps/BehatPhpServer/ApiServerContext.php#L249
Added line #L249 was not covered by tests
222-271: Overall well-implemented file response handling.The
apiWillRespondWithFilemethod is well-structured with appropriate error handling and content type detection. The PHPDoc is comprehensive with good examples.For binary files, the current implementation should work correctly with base64 encoding, but you might want to add a comment explaining how binary files are handled to make it clearer for future developers.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 240-244: src/DrevOps/BehatPhpServer/ApiServerContext.php#L240-L244
Added lines #L240 - L244 were not covered by tests
[warning] 249-249: src/DrevOps/BehatPhpServer/ApiServerContext.php#L249
Added line #L249 was not covered by tests
[warning] 259-259: src/DrevOps/BehatPhpServer/ApiServerContext.php#L259
Added line #L259 was not covered by tests
| if (!$file_found) { | ||
| throw new \RuntimeException(sprintf( | ||
| 'File "%s" does not exist in any of the configured fixture paths: %s', | ||
| $file_path, | ||
| implode(', ', $error_paths) | ||
| )); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add tests for file not found error path.
The static analysis shows these lines are not covered by tests. Consider adding a test case where a file is not found in any of the configured fixture paths.
🏁 Script executed:
#!/bin/bash
# Look for any tests that cover the file not found scenario
echo "Searching for tests that might cover file not found scenario..."
rg -A 5 "assertThrows|catch|exception|error" tests/ | grep -i "file.*not.*found\|not.*exist"
echo "Checking if there are any negative test cases for apiWillRespondWithFile..."
rg -A 10 "apiWillRespondWithFile" tests/Length of output: 560
Action: Add tests to cover file-not-found error scenarios in ApiServerContext.
The static analysis indicates that the exception thrown when a file isn’t found (lines 239–245 in src/DrevOps/BehatPhpServer/ApiServerContext.php) isn’t exercised by any test. The current test results show no negative test cases—specifically for the method (e.g., apiWillRespondWithFile)—that trigger this error path.
Recommendations:
- Add a test case in your suite (for example, in
tests/phpunit/Unit/ApiServerContextTest.php) that simulates a missing file, ensuring that the expected\RuntimeExceptionis thrown with the correct message. - Verify that all fixture paths are being checked correctly and that the error paths are covered.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 240-244: src/DrevOps/BehatPhpServer/ApiServerContext.php#L240-L244
Added lines #L240 - L244 were not covered by tests
Summary by CodeRabbit
New Features
Documentation
Tests