Add ability to check that the file is downloaded and appears in the e…#6184
Add ability to check that the file is downloaded and appears in the e…#6184TatianaTochko wants to merge 1 commit intovividus-framework:masterfrom
Conversation
📝 WalkthroughWalkthroughAdds a new step to assert a file exists at a resolved path, converts field injection to constructor injection in FileSteps, introduces a FILE constant, updates documentation for file-existence and ZIP archive entries, and adds unit and integration tests covering valid, missing, invalid, and malformed path scenarios. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Story
participant FileSteps
participant FS as Filesystem
participant SoftAssert as ISoftAssert
Note over Story,FileSteps: Scenario may create a file then assert its existence
User->>Story: run "create temp file" step
Story->>FileSteps: createFile(tempPath, content)
FileSteps->>FS: write file at tempPath
FS-->>FileSteps: write result
User->>Story: run "file exists at path" step
Story->>FileSteps: assertPathExists(tempPath)
alt input is null/blank
FileSteps->>SoftAssert: recordFailedAssertion("path must not be blank")
else input is invalid (InvalidPathException)
FileSteps->>SoftAssert: recordFailedAssertion("Invalid path: ...")
else valid path
FileSteps->>FS: isRegularFile(tempPath)?
alt file exists and is regular
FileSteps->>SoftAssert: recordPassedAssertion("File '" + tempPath + "' exists")
else missing or not regular
FileSteps->>SoftAssert: recordFailedAssertion("File '" + tempPath + "' does not exist")
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
🔇 Additional comments (6)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
docs/modules/commons/pages/vividus-steps.adoc(1 hunks)vividus-tests/src/main/resources/story/integration/FileValidationSteps.story(1 hunks)vividus/src/main/java/org/vividus/steps/FileSteps.java(3 hunks)vividus/src/test/java/org/vividus/steps/FileStepsTests.java(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (macos-latest)
- GitHub Check: system-kafka-tests
🔇 Additional comments (6)
vividus/src/main/java/org/vividus/steps/FileSteps.java (2)
24-24: LGTM!The new imports for
PathsandISoftAssertare necessary for the file existence validation functionality.Also applies to: 32-32
41-41: LGTM!The
ISoftAssertinjection follows the existing pattern and is properly configured for dependency injection.docs/modules/commons/pages/vividus-steps.adoc (1)
448-471: LGTM!The documentation for the File Existence Validation step is clear and comprehensive, with good examples covering both static and dynamic filename scenarios. The examples align well with the integration test scenarios.
vividus-tests/src/main/resources/story/integration/FileValidationSteps.story (1)
1-12: LGTM!The integration test scenarios provide good coverage of the file existence validation step, including both static and dynamic filename patterns. The use of the
generateDateexpression in the dynamic scenario demonstrates practical usage with runtime-generated values.vividus/src/test/java/org/vividus/steps/FileStepsTests.java (2)
47-48: LGTM!The introduction of
FILE_CONTENTandFILE_NAMEconstants improves test maintainability and reduces duplication.
86-118: Good test coverage with proper isolation.The new test methods provide good coverage of the file existence validation functionality, including edge cases like nested directories. The use of
@TempDirensures proper test isolation. However, as noted in a separate comment, these tests should also verify thesoftAssertinteractions.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6184 +/- ##
=========================================
Coverage 97.78% 97.78%
Complexity 7369 7369
=========================================
Files 1010 1010
Lines 21336 21350 +14
Branches 1398 1399 +1
=========================================
+ Hits 20863 20877 +14
Misses 357 357
Partials 116 116 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
vividus/src/main/java/org/vividus/steps/FileSteps.java(3 hunks)vividus/src/test/java/org/vividus/steps/FileStepsTests.java(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: system-kafka-tests
- GitHub Check: build (macos-latest)
- GitHub Check: build (windows-latest)
|
@TatianaTochko please fix the failing builds |
TatianaTochko
force-pushed
the
feature/6015
branch
from
71b2209 to
eef6f85
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
docs/modules/commons/pages/vividus-steps.adoc(1 hunks)vividus-tests/src/main/resources/story/integration/FileValidationSteps.story(1 hunks)vividus/src/main/java/org/vividus/steps/FileSteps.java(4 hunks)vividus/src/test/java/org/vividus/steps/FileStepsTests.java(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (macos-latest)
- GitHub Check: system-kafka-tests
🔇 Additional comments (5)
vividus/src/main/java/org/vividus/steps/FileSteps.java (1)
82-100: LGTM! Well-implemented file existence validation.The implementation correctly addresses all previous review feedback:
- Method is now void (appropriate for a Then-step)
- Null checks prevent NPEs with clear error messages
- Soft assertion provides proper test feedback
- Formatting is clean and readable
The path construction using
Paths.get(filePath, fileName)correctly combines the directory path with the filename.vividus/src/test/java/org/vividus/steps/FileStepsTests.java (2)
47-50: Good use of constants for test data.The introduction of well-named constants (
FILE_CONTENT,FILE_EXIST,FILE_NAME,FILE_NAME_AND_FILE_PATH_MUST_NOT_BE_NULL) improves test maintainability and reduces duplication.
104-115: Rename test to reflect void method behavior.The test name
shouldReturnTrueWhenFileExistssuggests the method returns a value, butdoesFileExistis now void. The test correctly verifies the soft assertion behavior.Consider renaming to reflect the assertion behavior:
- void shouldReturnTrueWhenFileExists(@TempDir Path tempDir) throws IOException + void shouldAssertTrueWhenFileExists(@TempDir Path tempDir) throws IOExceptionLikely an incorrect or invalid review comment.
vividus-tests/src/main/resources/story/integration/FileValidationSteps.story (1)
1-12: Excellent integration test coverage.The story provides good coverage of the new file existence validation step:
- Static filename scenario (lines 5-7): Tests straightforward file validation
- Dynamic filename scenario (lines 9-12): Tests with variable interpolation using
generateDateexpressionBoth scenarios follow the logical pattern of creating a file first, then validating its existence. The meta tags appropriately categorize the story.
docs/modules/commons/pages/vividus-steps.adoc (1)
448-471: Well-documented step with clear examples.The documentation provides:
- Clear step syntax matching the implementation
- Descriptive parameter explanations
- Two practical examples: static filename and dynamic filename with variable interpolation
- Consistent formatting with the rest of the document
The examples align with the integration test scenarios in
FileValidationSteps.story, demonstrating good coordination between code, tests, and documentation.
TatianaTochko
force-pushed
the
feature/6015
branch
from
eef6f85 to
8f47392
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
docs/modules/commons/pages/vividus-steps.adoc(1 hunks)vividus-tests/src/main/resources/story/integration/FileValidationSteps.story(1 hunks)vividus/src/main/java/org/vividus/steps/FileSteps.java(4 hunks)vividus/src/test/java/org/vividus/steps/FileStepsTests.java(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: system-kafka-tests
- GitHub Check: build (macos-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (windows-latest)
🔇 Additional comments (3)
vividus/src/main/java/org/vividus/steps/FileSteps.java (1)
82-100: LGTM! Implementation addresses previous review feedback.The implementation correctly:
- Uses void return type for a Then-step
- Includes defensive null checks with clear error messages
- Constructs the full path using standard Java NIO
- Performs soft assertions with descriptive messages
vividus/src/test/java/org/vividus/steps/FileStepsTests.java (2)
47-50: Well-structured test constants improve maintainability.The introduction of test constants (
FILE_CONTENT,FILE_EXIST,FILE_NAME,FILE_NAME_AND_FILE_PATH_MUST_NOT_BE_NULL) eliminates magic strings and makes the tests more maintainable.
88-127: Comprehensive test coverage for the new validation step.The test suite properly covers:
- Null parameter validation for both
fileNameandfilePath- Successful file existence verification with ArgumentCaptor
- Failed verification when file doesn't exist
- Proper verification of soft assertion interactions
The use of
ArgumentCaptorto verify both the message format and boolean values is a best practice for testing assertion behavior.
TatianaTochko
force-pushed
the
feature/6015
branch
from
8f47392 to
4bdf9e0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
vividus/src/test/java/org/vividus/steps/FileStepsTests.java (2)
117-127: Consider renaming test to reflect void method behavior.Similar to the previous test,
shouldReturnFalseWhenFileDoesNotExistimplies a return value. Consider a name that better reflects the assertion behavior.Suggested rename:
- void shouldReturnFalseWhenFileDoesNotExist(@TempDir Path tempDir) + void shouldAssertFalseWhenFileDoesNotExist(@TempDir Path tempDir)or
- void shouldReturnFalseWhenFileDoesNotExist(@TempDir Path tempDir) + void shouldVerifyFileMissingWhenFileDoesNotExist(@TempDir Path tempDir)
104-115: Consider renaming test to reflect void method behavior.The test name
shouldReturnTrueWhenFileExistsimplies a return value, but the method is void and uses soft assertions. Consider a name that better reflects the assertion behavior.Suggested rename:
- void shouldReturnTrueWhenFileExists(@TempDir Path tempDir) throws IOException + void shouldAssertTrueWhenFileExists(@TempDir Path tempDir) throws IOExceptionor
- void shouldReturnTrueWhenFileExists(@TempDir Path tempDir) throws IOException + void shouldVerifyFileExistsWhenFileIsPresent(@TempDir Path tempDir) throws IOExceptionvividus-tests/src/main/resources/story/integration/FileValidationSteps.story (1)
5-8: Missing variable initialization will cause test failure.Line 7 references
${temp-file-content}but this variable is never initialized in the scenario. This will either:
- Cause the step to fail if the variable is required
- Create a file containing the literal text
${temp-file-content}instead of actual contentApply this diff to initialize the variable:
Scenario: Verify file existence validation Given I initialize scenario variable `test-dir` with value `${java.io.tmpdir}` +Given I initialize scenario variable `temp-file-content` with value `Sample test content` When I create file with content `${temp-file-content}` at path `${test-dir}/test-file.txt` Then file with name `test-file.txt` exists by path `${test-dir}`
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
docs/modules/commons/pages/vividus-steps.adoc(1 hunks)vividus-tests/src/main/resources/story/integration/FileValidationSteps.story(1 hunks)vividus/src/main/java/org/vividus/steps/FileSteps.java(4 hunks)vividus/src/test/java/org/vividus/steps/FileStepsTests.java(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: system-kafka-tests
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (macos-latest)
🔇 Additional comments (9)
vividus/src/main/java/org/vividus/steps/FileSteps.java (3)
2-2: LGTM: Copyright year updated.The copyright year has been correctly updated to 2025.
24-24: LGTM: Required imports and dependency injection.The necessary imports (
Paths,Then,ISoftAssert) and the injectedISoftAssertfield have been correctly added to support the new file existence validation step.Also applies to: 29-29, 32-32, 41-41
82-100: LGTM: File existence validation step implemented correctly.The implementation addresses all previous review feedback:
- Method is void (no longer returns boolean)
- Javadoc is correct (no @return tag)
- Null checks are in place with appropriate error messages
- Path construction and file existence check are correct
- Soft assertion provides clear error message
docs/modules/commons/pages/vividus-steps.adoc (1)
448-459: LGTM: Clear documentation for the new step.The documentation clearly describes the new file existence validation step, including parameter descriptions and usage.
vividus/src/test/java/org/vividus/steps/FileStepsTests.java (5)
2-2: LGTM: Copyright year updated.The copyright year has been correctly updated to 2025.
23-23: LGTM: Test infrastructure properly configured.The necessary imports (
verifyNoMoreInteractions,ArgumentCaptor) and theISoftAssertmock have been correctly added to support testing the new file existence validation behavior.Also applies to: 36-36, 41-41, 55-56
47-50: LGTM: Test constants improve maintainability.The introduction of constants for test data (
FILE_CONTENT,FILE_EXIST,FILE_NAME,FILE_NAME_AND_FILE_PATH_MUST_NOT_BE_NULL) enhances readability and reduces duplication across tests.
61-78: LGTM: Existing tests updated to use constants.The existing tests have been correctly updated to use the new constants instead of inline literals, improving consistency and maintainability.
Also applies to: 80-86
88-94: LGTM: Null parameter validation tests are thorough.Both null check tests correctly verify that:
softAssert.recordFailedAssertionis called with the expected message- No other interactions occur with the soft assert mock
This ensures the early-return behavior works as expected.
Also applies to: 96-102
TatianaTochko
force-pushed
the
feature/6015
branch
2 times, most recently
from
7c5d7fa to
bc57aac
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
vividus/src/test/java/org/vividus/steps/FileStepsTests.java (1)
104-127: Consider renaming test methods to reflect void behavior.The test method names
shouldReturnTrueWhenFileExistsandshouldReturnFalseWhenFileDoesNotExistare misleading sincedoesFileExistis a void method that performs assertions rather than returning values. The "shouldReturn" prefix suggests a return value.Consider renaming to better reflect the assertion behavior:
- void shouldReturnTrueWhenFileExists(@TempDir Path tempDir) throws IOException + void shouldAssertTrueWhenFileExists(@TempDir Path tempDir) throws IOException- void shouldReturnFalseWhenFileDoesNotExist(@TempDir Path tempDir) + void shouldAssertFalseWhenFileDoesNotExist(@TempDir Path tempDir)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
docs/modules/commons/pages/vividus-steps.adoc(1 hunks)vividus-tests/src/main/resources/story/integration/FileValidationSteps.story(1 hunks)vividus/src/main/java/org/vividus/steps/FileSteps.java(4 hunks)vividus/src/test/java/org/vividus/steps/FileStepsTests.java(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (macos-latest)
- GitHub Check: system-kafka-tests
🔇 Additional comments (6)
vividus-tests/src/main/resources/story/integration/FileValidationSteps.story (1)
5-9: LGTM! The integration scenario is correctly structured.The scenario properly initializes the test directory, creates a file, and validates its existence. The past path construction issue has been successfully resolved by using the system temp directory variable and passing it correctly to the validation step.
vividus/src/main/java/org/vividus/steps/FileSteps.java (1)
2-2: LGTM! Copyright year updated appropriately.docs/modules/commons/pages/vividus-steps.adoc (1)
448-467: LGTM! Documentation is clear and complete.The File Existence Validation documentation accurately describes the new step with a complete, working example that properly initializes all required variables and demonstrates the correct usage pattern.
vividus/src/test/java/org/vividus/steps/FileStepsTests.java (3)
2-2: LGTM! Copyright year updated appropriately.
47-50: LGTM! Well-defined test constants improve maintainability.The constants are appropriately scoped and clearly named, making the tests more readable and maintainable.
88-102: LGTM! Comprehensive null parameter validation.Both null check tests properly verify that the soft assertion records the failure and that no additional interactions occur. Good defensive test coverage.
TatianaTochko
force-pushed
the
feature/6015
branch
2 times, most recently
from
7270b8d to
2ca0ebf
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
docs/modules/commons/pages/vividus-steps.adoc(1 hunks)vividus-tests/src/main/resources/story/integration/FileValidationSteps.story(1 hunks)vividus/src/main/java/org/vividus/steps/FileSteps.java(4 hunks)vividus/src/test/java/org/vividus/steps/FileStepsTests.java(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build (windows-latest)
- GitHub Check: build (macos-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: system-kafka-tests
🔇 Additional comments (9)
vividus/src/test/java/org/vividus/steps/FileStepsTests.java (5)
2-2: LGTM!The copyright update, new imports, constants, and mock setup are appropriate for the new test functionality. The constants improve test maintainability and readability.
Also applies to: 23-23, 36-36, 41-41, 47-57
62-87: Good refactoring to use constants.The existing tests have been appropriately updated to use the new constants, improving maintainability without changing test behavior.
89-103: Excellent null parameter validation tests.The tests properly verify that null parameters are caught early with descriptive error messages, and correctly use
verifyNoMoreInteractionsto ensure no additional assertions are performed.
105-116: Well-structured file existence test.The test correctly creates an actual file, verifies the soft assertion is called with the expected message and boolean value using ArgumentCaptor. Good use of @tempdir for automatic cleanup.
118-128: Correct negative test case.The test properly validates the scenario when a file does not exist, verifying that the soft assertion receives the expected message and false boolean value.
vividus-tests/src/main/resources/story/integration/FileValidationSteps.story (1)
1-9: LGTM!The integration story correctly demonstrates the file existence validation feature. The path construction is correct: the file is created at
${test-dir}/test-file.txtand validated by checking for filenametest-file.txtat directory${test-dir}.vividus/src/main/java/org/vividus/steps/FileSteps.java (2)
2-2: LGTM!The copyright update, new imports, and ISoftAssert injection are appropriate for the new file existence validation functionality.
Also applies to: 24-24, 29-29, 32-32, 41-41
82-105: Well-implemented file existence validation step.The method properly:
- Validates null inputs with specific error messages
- Constructs the full path using
Paths.get(filePath, fileName)- Uses soft assertions with descriptive messaging
- Follows the existing codebase patterns
docs/modules/commons/pages/vividus-steps.adoc (1)
448-459: Good documentation structure and clarity.The documentation clearly explains the new file existence validation step with appropriate parameter descriptions.
TatianaTochko
force-pushed
the
feature/6015
branch
3 times, most recently
from
2045e82 to
ebcb6d8
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
docs/modules/commons/pages/vividus-steps.adoc(1 hunks)vividus-tests/src/main/resources/story/integration/FileValidationSteps.story(1 hunks)vividus/src/main/java/org/vividus/steps/FileSteps.java(4 hunks)vividus/src/test/java/org/vividus/steps/FileStepsTests.java(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (windows-latest)
- GitHub Check: build (macos-latest)
- GitHub Check: system-kafka-tests
🔇 Additional comments (5)
vividus/src/main/java/org/vividus/steps/FileSteps.java (1)
24-33: LGTM: proper dependencies and injection.Adding Paths and injecting ISoftAssert is appropriate for the new step.
Also applies to: 41-41
vividus-tests/src/main/resources/story/integration/FileValidationSteps.story (1)
5-9: Scenario looks correct and robust.Uses ${java.io.tmpdir}, writes the file there, and validates existence against the directory path.
vividus/src/test/java/org/vividus/steps/FileStepsTests.java (2)
90-136: Good negative-argument coverage with precise messages.Null/empty/blank cases are verified and guarded with verifyNoMoreInteractions; concise and effective.
82-88: Remove the suggested change; the test is already correct.FileUtils.writeStringToFile automatically creates the file's parent directories if they do not exist (this behavior has been in Commons‑IO since v1.3). The codebase uses commons-io 2.20.0, which is well after v1.3, so the test will create the "temp" parent directory without explicit intervention. The suggested createDirectories() call is unnecessary.
Likely an incorrect or invalid review comment.
docs/modules/commons/pages/vividus-steps.adoc (1)
460-467: Example reads well and matches the step behavior.Uses ${java.io.tmpdir} and validates against the directory; good.
TatianaTochko
force-pushed
the
feature/6015
branch
from
ebcb6d8 to
feb6033
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
docs/modules/commons/pages/vividus-steps.adoc (1)
457-457: Fix incorrect parameter documentation:$filePathis the full file path, not a directory.The documentation states "
$filePath- The directory path where the file should exist", but this is incorrect. Based on the implementation, example, and past review feedback,$filePathshould be the full path to the file (including the filename), not just the directory.Apply this diff to correct the documentation:
-* `$filePath` - The directory path where the file should exist +* `$filePath` - The full path to the file to check for existenceBased on past review comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
docs/modules/commons/pages/vividus-steps.adoc(1 hunks)vividus-tests/src/main/resources/story/integration/FileValidationSteps.story(1 hunks)vividus/src/main/java/org/vividus/steps/FileSteps.java(4 hunks)vividus/src/test/java/org/vividus/steps/FileStepsTests.java(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build (macos-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (windows-latest)
- GitHub Check: system-kafka-tests
🔇 Additional comments (2)
vividus-tests/src/main/resources/story/integration/FileValidationSteps.story (1)
5-9: LGTM! Integration test correctly exercises the new file existence validation step.The scenario properly initializes the test directory, creates a file, and verifies its existence using the full file path consistently.
docs/modules/commons/pages/vividus-steps.adoc (1)
459-466: LGTM! The example correctly demonstrates the file existence validation step.The example properly shows initializing the directory variable, creating a file with content, and validating its existence using the full file path. This is consistent with the implementation and integration test.
TatianaTochko
force-pushed
the
feature/6015
branch
from
feb6033 to
dd00a6b
Compare
TatianaTochko
force-pushed
the
feature/6015
branch
from
5b92d8d to
1034b55
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
docs/modules/commons/pages/vividus-steps.adoc(1 hunks)vividus-tests/src/main/resources/story/integration/File validation steps.story(1 hunks)vividus/src/main/java/org/vividus/steps/FileSteps.java(3 hunks)vividus/src/test/java/org/vividus/steps/FileStepsTests.java(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: system-kafka-tests
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (windows-latest)
- GitHub Check: build (macos-latest)
TatianaTochko
force-pushed
the
feature/6015
branch
from
1034b55 to
269becc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
vividus/src/main/java/org/vividus/steps/FileSteps.java (2)
42-43: Consider using constructor-based injection.Both dependencies use field injection. Constructor-based injection is generally preferred as it provides better testability, immutability, and makes dependencies explicit.
Based on learnings (valfirst's comment).
Apply this refactor:
- @Inject private VariableContext variableContext; - @Inject private ISoftAssert softAssert; + private final VariableContext variableContext; + private final ISoftAssert softAssert; + + @Inject + public FileSteps(VariableContext variableContext, ISoftAssert softAssert) + { + this.variableContext = variableContext; + this.softAssert = softAssert; + }
84-87: Align Javadoc with documentation for clarity.The Javadoc says "Checks if a file exists" but the documentation (line 450 of vividus-steps.adoc) more accurately states "Checks that a regular file exists at the given path." The distinction is important since directories would fail this check.
Based on learnings (valfirst's comment).
Apply this diff:
/** - * Checks if a file exists at the given path + * Checks that a regular file exists at the given path * @param filePath The path where the file should exist */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
docs/modules/commons/pages/vividus-steps.adoc(1 hunks)vividus-tests/src/main/resources/story/integration/File validation steps.story(1 hunks)vividus/src/main/java/org/vividus/steps/FileSteps.java(3 hunks)vividus/src/test/java/org/vividus/steps/FileStepsTests.java(3 hunks)
🔇 Additional comments (7)
vividus-tests/src/main/resources/story/integration/File validation steps.story (1)
1-8: LGTM!The integration test story correctly validates the new file existence check. The scenario follows a clear pattern: initializes a file path, creates the file, and verifies its existence.
vividus/src/main/java/org/vividus/steps/FileSteps.java (1)
89-119: Well-structured implementation with comprehensive validation.The method correctly handles all edge cases:
- Null/blank path validation with early return
- InvalidPathException handling with descriptive error messages
- Clear distinction between "does not exist" and "is not a regular file" errors
- Appropriate use of soft assertions for test reporting
docs/modules/commons/pages/vividus-steps.adoc (1)
448-465: Excellent documentation with clear examples.The new "File existence validation" section is well-structured:
- Clearly specifies "regular file" validation (distinguishing from directories)
- Parameter description accurately indicates full path requirement
- Example is practical and matches the integration test story
- Consistent formatting with the rest of the document
vividus/src/test/java/org/vividus/steps/FileStepsTests.java (4)
54-58: Good use of constants for test maintainability.The extracted constants improve readability and make it easier to maintain consistent test data across multiple test methods.
96-105: Excellent use of parameterized test for edge cases.The parameterized test efficiently covers null, empty, and blank path scenarios in a single, maintainable test method.
107-116: Comprehensive validation of the happy path.The test correctly verifies both the recorded assertion message and ensures no additional soft-assert interactions occur.
126-152: Thorough coverage of edge cases.These tests effectively cover:
- Directory paths that should fail the "regular file" check
- Invalid paths that trigger
InvalidPathExceptionThe use of
MockedStaticto simulate the exception is appropriate and demonstrates good testing practices.
TatianaTochko
force-pushed
the
feature/6015
branch
from
269becc to
0fe8e3d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
vividus/src/main/java/org/vividus/steps/FileSteps.java (1)
84-87: Address the unresolved review comment: align Javadoc with documentation.The Javadoc should mention "regular file" to match the documentation and the actual behavior of the method (which uses
Files.isRegularFile()).Apply this diff to align the Javadoc:
/** - * Checks if a file exists at the given path + * Checks that a regular file exists at the given path * @param filePath The path where the file should exist */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
docs/modules/commons/pages/vividus-steps.adoc(1 hunks)vividus-tests/src/main/resources/story/integration/File validation steps.story(1 hunks)vividus/src/main/java/org/vividus/steps/FileSteps.java(3 hunks)vividus/src/test/java/org/vividus/steps/FileStepsTests.java(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: system-kafka-tests
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (macos-latest)
🔇 Additional comments (8)
vividus-tests/src/main/resources/story/integration/File validation steps.story (1)
1-8: LGTM! Clear integration test for the new feature.The integration story correctly demonstrates the file existence validation feature, using a temporary file path, creating the file, and asserting its existence. This provides good end-to-end coverage.
vividus/src/main/java/org/vividus/steps/FileSteps.java (1)
89-119: LGTM! Comprehensive validation logic.The implementation correctly handles all edge cases:
- Validates null/blank input with clear error message
- Catches and reports
InvalidPathExceptionfor malformed paths- Distinguishes between non-existent paths and non-regular files (directories)
- Records appropriate passed/failed assertions
The logic is solid and aligns with the test coverage.
docs/modules/commons/pages/vividus-steps.adoc (1)
448-465: LGTM! Clear and accurate documentation.The documentation correctly describes the new file existence validation feature:
- Clearly states it checks for a "regular file" (not just any path)
- Parameter description accurately specifies the full path including filename
- Example is practical and demonstrates the feature correctly
vividus/src/test/java/org/vividus/steps/FileStepsTests.java (5)
22-64: LGTM! Well-organized test setup.The new imports and constants are well-structured:
- Necessary imports for parameterized tests and static mocking
- Clear, reusable test constants
- Proper mock declaration for
ISoftAssert
69-94: LGTM! Good refactoring to use constants.The existing tests have been updated to use the new constants (
FILE_NAME,FILE_CONTENT), improving maintainability without changing test logic.
96-105: LGTM! Comprehensive parameterized test for invalid inputs.The parameterized test efficiently covers null, empty, and blank path scenarios with proper verification of the soft assertion behavior and no unexpected interactions.
107-136: LGTM! Thorough coverage of path existence scenarios.The tests comprehensively cover:
- Regular file exists → passed assertion
- Path does not exist → failed assertion with "does not exist" message
- Path is a directory → failed assertion with "is not a regular file" message
Each test properly uses
@TempDir, verifies the exact assertion message, and confirms no unexpected interactions.
138-153: LGTM! Effective test for exception handling.The test cleverly uses
MockedStaticto simulateInvalidPathExceptionand verifies that the catch block properly records a failed assertion with the exception details. This completes the test coverage for all code paths.
TatianaTochko
force-pushed
the
feature/6015
branch
from
0fe8e3d to
66bd5c3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
vividus/src/test/java/org/vividus/steps/FileStepsTests.java(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build (windows-latest)
- GitHub Check: build (macos-latest)
- GitHub Check: system-kafka-tests
🔇 Additional comments (4)
vividus/src/test/java/org/vividus/steps/FileStepsTests.java (4)
55-59: LGTM!The test constants are well-defined and improve test readability by avoiding string duplication.
64-65: LGTM!The ISoftAssert mock is properly configured and all test methods correctly verify the soft assertion interactions as indicated in the past review comments.
109-138: LGTM!The path existence validation tests comprehensively cover positive cases (file exists), negative cases (path doesn't exist), and edge cases (path is a directory). All softAssert interactions are properly verified with
verifyNoMoreInteractions.
140-155: LGTM!The use of
MockedStatic<Path>to simulateInvalidPathExceptionis an effective approach to test the exception handling path. The test properly verifies both the soft assertion behavior and the static method invocation.
TatianaTochko
force-pushed
the
feature/6015
branch
from
9f53e25 to
f064ba0
Compare
TatianaTochko
force-pushed
the
feature/6015
branch
from
f064ba0 to
22ca374
Compare
TatianaTochko
force-pushed
the
feature/6015
branch
from
22ca374 to
ed4aae3
Compare
TatianaTochko
force-pushed
the
feature/6015
branch
from
ed4aae3 to
7d8c48a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
docs/modules/commons/pages/vividus-steps.adoc(1 hunks)vividus-tests/src/main/resources/story/integration/File validation steps.story(1 hunks)vividus/src/main/java/org/vividus/steps/FileSteps.java(3 hunks)vividus/src/test/java/org/vividus/steps/FileStepsTests.java(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build (windows-latest)
- GitHub Check: build (macos-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: system-kafka-tests
🔇 Additional comments (9)
docs/modules/commons/pages/vividus-steps.adoc (1)
475-492: LGTM! Documentation is clear and complete.The new file existence validation section is well-documented with clear parameter descriptions and a practical example. The documentation accurately reflects the implementation and provides users with sufficient information to use the step correctly.
vividus/src/main/java/org/vividus/steps/FileSteps.java (3)
1-36: LGTM! Imports and copyright properly updated.The copyright year has been updated to 2025, and all new imports are necessary for the new file existence validation functionality.
39-48: Excellent implementation of constructor-based injection.The migration from field injection to constructor injection follows best practices. The fields are properly marked as final, and the constructor cleanly initializes both dependencies. The new
FILEconstant promotes consistency in error messages.
89-117: Add test case and clarify whether directories should be accepted.The method uses
Files.exists(path)which returnstruefor any path type (files, directories, symlinks). However, the Javadoc and parameter example suggest regular files only. The test suite covers only regular files and has no test case for directories.Either:
- Add a test confirming that directories should pass (and update the Javadoc to reflect this), or
- Use
Files.isRegularFile(path)to reject directories and update the Javadoc accordingly.vividus/src/test/java/org/vividus/steps/FileStepsTests.java (5)
1-62: LGTM! Test setup follows best practices.The test class has been properly updated with constructor injection support, appropriate mocking of
ISoftAssert, and well-defined constants for test data. The new imports support parameterized testing for comprehensive validation coverage.
64-89: LGTM! Existing tests properly refactored.The existing test methods have been correctly updated to use the new constants, improving maintainability and reducing duplication. The exception handling in the lambda is appropriate.
91-101: LGTM! Comprehensive parameterized test for invalid inputs.The parameterized test effectively covers null, empty, and blank path inputs, verifying that
IllegalArgumentExceptionis thrown with the correct message. This provides robust validation of the precondition checks.
103-122: Good test coverage for basic success and failure scenarios.The tests properly verify both the success path (file exists) and failure path (file doesn't exist) with correct soft-assert interactions. The use of
verifyNoMoreInteractionsensures no unexpected assertion calls occur.Consider adding a test case to verify behavior when a directory path is provided, especially since
Files.exists()returnstruefor directories. This would clarify the intended semantics and prevent regressions:@Test void testShouldPassOrFailWhenPathIsDirectory(@TempDir Path tempDir) { Path directoryPath = tempDir.resolve("test-directory"); Files.createDirectory(directoryPath); fileSteps.assertPathExists(directoryPath.toString()); // Verify expected behavior: should it pass or fail for directories? verify(softAssert).recordPassedAssertion(...); // or recordFailedAssertion? }
124-131: LGTM! Excellent test for invalid path handling.The test effectively exercises the
InvalidPathExceptioncatch block using a null byte character, which is universally invalid across platforms. The use ofstartsWithmatcher appropriately handles platform-specific exception messages.
TatianaTochko
force-pushed
the
feature/6015
branch
from
7d8c48a to
bda5950
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
vividus/src/main/java/org/vividus/steps/FileSteps.java (1)
110-117: UseFiles.isRegularFile()to match the step's file-specific semantics.The step is named "file exists at path" and the documentation states it "ensures that a file exists." However,
Files.exists()returns true for directories, symbolic links, and other non-regular file types. This semantic mismatch could lead to false positives when a directory exists at the given path instead of a regular file.Apply this diff to ensure the step validates regular files only and provides clear error messages:
- if (!Files.exists(path)) + if (!Files.exists(path)) { softAssert.recordFailedAssertion(FILE + path + "' does not exist"); return; } - - softAssert.recordPassedAssertion(FILE + path + "' exists"); + if (!Files.isRegularFile(path)) + { + softAssert.recordFailedAssertion(FILE + path + "' exists but is not a regular file"); + return; + } + softAssert.recordPassedAssertion(FILE + path + "' exists");This change ensures that:
- Missing paths are reported as "does not exist"
- Directories and other non-regular paths are reported as "not a regular file"
- Only regular files pass the assertion
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
docs/modules/commons/pages/vividus-steps.adoc(1 hunks)vividus-tests/src/main/resources/story/integration/File validation steps.story(1 hunks)vividus/src/main/java/org/vividus/steps/FileSteps.java(3 hunks)vividus/src/test/java/org/vividus/steps/FileStepsTests.java(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build (macos-latest)
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: system-kafka-tests
🔇 Additional comments (5)
vividus/src/main/java/org/vividus/steps/FileSteps.java (2)
39-48: LGTM! Constructor injection implemented correctly.The migration from field injection to constructor injection follows best practices. The FILE constant provides consistent messaging across assertions.
89-108: Input validation and exception handling are correct.The validation logic properly handles null/empty/blank inputs using
Validate.isTrueand catchesInvalidPathExceptiongracefully with clear error messages.vividus/src/test/java/org/vividus/steps/FileStepsTests.java (3)
52-59: Good use of constants and proper mock setup.The introduction of
FILE_CONTENTandFILE_NAMEconstants eliminates duplication across tests. TheISoftAssertmock is properly configured for verifying assertion behavior.
91-101: Comprehensive validation test for null/empty/blank paths.The parameterized test effectively covers all invalid input scenarios (null, empty, blank) and correctly expects
IllegalArgumentExceptionwith the appropriate message.
124-131: Invalid path exception handling test is well-designed.The test effectively triggers
InvalidPathExceptionusing a null character and properly verifies the error message format and that no additional interactions occur.
TatianaTochko
force-pushed
the
feature/6015
branch
from
bda5950 to
034d415
Compare
There was a problem hiding this comment.
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…xpected folder
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.