[#1560] Replaced BATS with PHPUnit for functional internal Vortex tests. #1657
Conversation
|
Note Reviews pausedUse the following commands to manage reviews:
WalkthroughThe changes introduce a comprehensive PHPUnit-based functional testing suite for workflow validation, replacing equivalent BATS tests. Several new PHP classes and traits are added to support assertions, Git operations, logging, and stepwise test execution. Supporting configuration files for Composer, PHPUnit, PHPStan, PHPCS, and Rector are introduced. Environment variable logic for volume mounting is unified and updated across test scripts and CI workflows. Changes
Sequence Diagram(s)sequenceDiagram
participant CI
participant TestScript
participant PHPUnit
participant SUT (System Under Test)
participant Docker
CI->>TestScript: Set VORTEX_DEV_VOLUMES_SKIP_MOUNT=1
TestScript->>PHPUnit: Run Functional Workflow Tests
PHPUnit->>SUT: Prepare SUT (install, configure)
SUT->>Docker: Start containers, adjust for volume mounting
PHPUnit->>SUT: Run build step (ahoy build)
SUT->>Docker: Build project, sync files
PHPUnit->>SUT: Run BDD tests (ahoy test-bdd)
SUT->>Docker: Execute BDD tests, generate artifacts
PHPUnit->>SUT: Assert files, clean up
Assessment against linked issues
Suggested labels
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
AlexSkrypnyk
added
DO NOT MERGE
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #1657 +/- ##
========================================
Coverage 71.52% 71.52%
========================================
Files 76 76
Lines 4320 4320
========================================
Hits 3090 3090
Misses 1230 1230 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 34
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (4)
.vortex/tests/composer.lockis excluded by!**/*.lock.vortex/tests/package-lock.jsonis excluded by!**/package-lock.json.vortex/tests/phpunit/composer.lockis excluded by!**/*.lock.vortex/tests/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (26)
.circleci/config.yml(1 hunks).github/workflows/vortex-test-common.yml(3 hunks).vortex/tests/bats/_helper.bash(4 hunks).vortex/tests/bats/_helper.deployment.bash(1 hunks).vortex/tests/bats/_helper.workflow.bash(2 hunks).vortex/tests/bats/workflow.smoke.bats(1 hunks).vortex/tests/phpunit/composer.json(1 hunks).vortex/tests/phpunit/phpcs.xml(1 hunks).vortex/tests/phpunit/phpstan.neon(1 hunks).vortex/tests/phpunit/phpunit.xml(1 hunks).vortex/tests/phpunit/rector.php(1 hunks).vortex/tests/phpunit/tests/Functional/FunctionalTestCase.php(1 hunks).vortex/tests/phpunit/tests/Functional/WorkflowTest.php(1 hunks).vortex/tests/phpunit/tests/Traits/ArrayTrait.php(1 hunks).vortex/tests/phpunit/tests/Traits/AssertFilesTrait.php(1 hunks).vortex/tests/phpunit/tests/Traits/GitTrait.php(1 hunks).vortex/tests/phpunit/tests/Traits/LocationsTrait.php(1 hunks).vortex/tests/phpunit/tests/Traits/LoggerTrait.php(1 hunks).vortex/tests/phpunit/tests/Traits/ProcessTrait.php(1 hunks).vortex/tests/phpunit/tests/Traits/Steps/StepBuildTrait.php(1 hunks).vortex/tests/phpunit/tests/Traits/Steps/StepDownloadDbTrait.php(1 hunks).vortex/tests/phpunit/tests/Traits/Steps/StepPrepareSutTrait.php(1 hunks).vortex/tests/phpunit/tests/Traits/Steps/StepTestBddAllTrait.php(1 hunks).vortex/tests/phpunit/tests/Traits/Steps/StepTestBddTrait.php(1 hunks).vortex/tests/phpunit/tests/Unit/ArrayTraitTest.php(1 hunks).vortex/tests/test.workflow.sh(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
.vortex/tests/phpunit/tests/Traits/Steps/StepTestBddAllTrait.php (1)
.vortex/tests/phpunit/tests/Traits/LoggerTrait.php (3)
logStepStart(11-17)log(7-9)logStepFinish(19-25)
.vortex/tests/phpunit/tests/Traits/LoggerTrait.php (1)
.vortex/installer/src/Utils/Tui.php (1)
label(106-129)
.vortex/tests/phpunit/tests/Traits/Steps/StepTestBddTrait.php (4)
.vortex/installer/src/Utils/File.php (1)
File(9-46).vortex/tests/phpunit/tests/Traits/LoggerTrait.php (3)
logStepStart(11-17)logSubstep(27-29)logStepFinish(19-25).vortex/tests/phpunit/tests/Traits/ProcessTrait.php (1)
processRun(32-93).vortex/tests/phpunit/tests/Functional/FunctionalTestCase.php (2)
syncToHost(70-77)processRunInContainer(88-100)
.vortex/tests/phpunit/tests/Unit/ArrayTraitTest.php (1)
.vortex/tests/phpunit/tests/Traits/ArrayTrait.php (2)
assertArrayContainsArray(7-49)assertArrayNotContainsArray(51-90)
.vortex/tests/phpunit/tests/Functional/FunctionalTestCase.php (4)
.vortex/installer/src/Utils/File.php (1)
File(9-46).vortex/tests/phpunit/tests/Functional/WorkflowTest.php (1)
setUp(10-17).vortex/tests/phpunit/tests/Traits/LoggerTrait.php (1)
logSubstep(27-29).vortex/tests/phpunit/tests/Traits/ProcessTrait.php (1)
processRun(32-93)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: build (0)
- GitHub Check: build (1)
- GitHub Check: vortex-test-workflow (2)
- GitHub Check: vortex-test-workflow (3)
- GitHub Check: vortex-test-workflow (1)
🔇 Additional comments (30)
.github/workflows/vortex-test-common.yml (1)
31-31: Environment variable refactoring looks good.The change from
VORTEX_DEV_VOLUMES_MOUNTED=0toVORTEX_DEV_VOLUMES_SKIP_MOUNT=1is consistent across all three container job definitions and appears to be part of a coordinated refactoring effort across the codebase.Also applies to: 112-112, 172-172
.vortex/tests/test.workflow.sh (3)
28-28: Good addition of PHP dependency management.Adding the check for the
vendordirectory and installing PHP dependencies when missing is consistent with the existing Node.js dependency management pattern on line 27.
51-52: PHPUnit integration in workflow execution is well-structured.The addition of PHPUnit tests for unit tests and functional workflow tests in index 0 is appropriately placed before the existing BATS smoke tests, ensuring comprehensive test coverage.
41-45: 🧹 Nitpick (assertive)Consider consistency with the bats function pattern.
The
phpunitfunction changes to the root directory but doesn't usepushd/popdlike thebatsfunction does. This creates inconsistency in the implementation pattern.Consider maintaining consistency with the
batsfunction pattern:phpunit() { pushd "${ROOT_DIR}" >/dev/null || exit 1 - ./vendor/bin/phpunit "$@" + "${ROOT_DIR}/vendor/bin/phpunit" "$@" popd >/dev/null || exit 1 }Alternatively, if the current approach is preferred, consider updating the
batsfunction to match this simpler pattern.Likely an incorrect or invalid review comment.
.circleci/config.yml (1)
453-453: Consistent environment variable refactoring.The change from
VORTEX_DEV_VOLUMES_MOUNTED=0toVORTEX_DEV_VOLUMES_SKIP_MOUNT=1matches the pattern applied in the GitHub workflows, ensuring consistency across all CI configurations..vortex/tests/phpunit/phpstan.neon (1)
1-24: Well-configured PHPStan setup for strict static analysis.The configuration is well-structured with:
- Level 9 (strictest analysis) for maximum code quality enforcement
- Appropriate path targeting (
testsdirectory)- Sensible exclusions (
vendor/*,node_modules/*)- Reasonable ignore rule for test-specific scenarios where iterable parameter types don't need detailed documentation
This setup will help maintain high code quality in the new PHPUnit test suite.
.vortex/tests/bats/workflow.smoke.bats (1)
74-74: Consistent environment variable refactoring with proper logic inversion.The change from checking
VORTEX_DEV_VOLUMES_MOUNTED != "1"toVORTEX_DEV_VOLUMES_SKIP_MOUNT=1maintains the same behavioral logic while using the new environment variable naming convention. The default value of "0" ensures backward compatibility..vortex/tests/phpunit/tests/Traits/Steps/StepTestBddAllTrait.php (1)
5-16:Details
❓ Verification inconclusive
Add documentation and consider implementation status.
The trait lacks proper documentation and contains only a placeholder implementation. Consider:
- Add class-level docblock explaining the trait's purpose
- Add method-level docblock for
stepTestBddAll()- Clarify if this is intended as a stub or needs actual implementation
Apply this diff to improve documentation:
+/** + * Trait for BDD all tests step functionality. + */ trait StepTestBddAllTrait { + /** + * Execute BDD all tests step. + * + * @todo Implement actual BDD all tests logic. + */ protected function stepTestBddAll(): void {Is this trait intended to remain as a placeholder, or should it have actual BDD test implementation?
Add missing documentation and clarify implementation intent
This trait currently serves as a placeholder without any real BDD logic. To keep it consistent with the rest of the codebase, please:
- Add a class‐level docblock explaining the trait’s purpose and status
- Add a method‐level docblock for
stepTestBddAll()- Confirm whether this is intended purely as a stub (to be implemented later) or needs full BDD‐all‐tests logic now
Suggested diff for consistency (adjust wording to match existing conventions):
+/** + * Trait StepTestBddAllTrait + * + * Provides the “BDD all tests” step for test suites. + * + * @todo Clarify whether this is a placeholder for future + * BDD logic or requires a full implementation now. + */ trait StepTestBddAllTrait { + /** + * Execute the “BDD all tests” step. + * + * Logs the start and finish of the BDD‐all‐tests step. + * + * @todo Implement actual BDD all tests logic. + */ protected function stepTestBddAll(): void { $this->logStepStart(); // BDD all tests implementation can be added here $this->log('BDD all tests step completed'); $this->logStepFinish(); }Is this trait intended to remain as a stub for later, or should it implement the actual BDD‐all‐tests logic today?
.vortex/tests/bats/_helper.workflow.bash (2)
11-13: LGTM! Environment variable refactor with correct logic inversion.The change from
VORTEX_DEV_VOLUMES_MOUNTEDtoVORTEX_DEV_VOLUMES_SKIP_MOUNTwith inverted logic is implemented correctly:
- Old:
VORTEX_DEV_VOLUMES_MOUNTED=${VORTEX_DEV_VOLUMES_MOUNTED:-1}(default mounted)- New:
VORTEX_DEV_VOLUMES_SKIP_MOUNT=${VORTEX_DEV_VOLUMES_SKIP_MOUNT:-0}(default not skipped)This maintains the same behavior while using clearer naming.
526-527: LGTM! Strategic placement of sync calls for container/host synchronization.The explicit
sync_to_hostcalls are well-placed:
- Before cleaning up screenshots and test results
- After BDD test execution
This ensures proper file synchronization between container and host during the test workflow.
Also applies to: 532-532, 535-535
.vortex/tests/bats/_helper.deployment.bash (1)
125-125: LGTM! Consistent environment variable refactor with correct logic.The condition change maintains the same behavior with clearer variable naming:
- Old:
VORTEX_DEV_VOLUMES_MOUNTED != "1"(when not mounted)- New:
VORTEX_DEV_VOLUMES_SKIP_MOUNT = "1"(when skipping mount)This aligns with the coordinated refactor across multiple test helper files.
.vortex/tests/phpunit/phpcs.xml (1)
1-31: LGTM! Well-configured PHPCS ruleset for PHPUnit test suite.The configuration provides:
- ✅ Drupal coding standard as base (appropriate for this project)
- ✅ Strict types requirement for better type safety
- ✅ Reasonable test-specific exclusions (long arrays, missing function comments)
- ✅ Proper scope to
testsdirectory- ✅ Helpful output formatting (progress, colors, sniff codes)
This complements the broader PHPUnit migration effort and establishes good code quality standards for the new test suite.
.vortex/tests/phpunit/tests/Traits/LocationsTrait.php (1)
1-18: LGTM! Clean trait implementation with helpful documentation.The trait properly wraps the upstream
LocationsTraitand provides clear documentation for the location variables used throughout the test suite. This follows good practices for extending external dependencies while maintaining local documentation..vortex/tests/phpunit/phpunit.xml (1)
1-31: Excellent PHPUnit configuration with comprehensive settings.The configuration is well-structured with appropriate strict settings for a robust test suite:
- Proper schema validation and bootstrap setup
- Strict error handling and detailed output for debugging
- JUnit logging for CI integration
- Appropriate source filtering
This will provide a solid foundation for the new PHPUnit test suite.
.vortex/tests/phpunit/tests/Traits/Steps/StepTestBddTrait.php (1)
14-39: Well-structured BDD test orchestration with proper error handling.The method effectively:
- Implements retry logic for handling flaky BDD tests
- Syncs files between container and host appropriately
- Validates expected test artifacts (screenshots and results)
- Performs comprehensive cleanup of both host and container files
The step-by-step logging provides good visibility into the test execution process.
.vortex/tests/phpunit/tests/Functional/WorkflowTest.php (1)
19-31: Excellent idempotence test strategy.The test effectively validates that:
- The build process produces trackable results in Git
- BDD tests run successfully on the initial build
- Rebuilding produces identical results (idempotence)
- BDD tests continue to pass after rebuild
This comprehensive workflow testing approach will help ensure the reliability of the build process.
.vortex/tests/phpunit/rector.php (1)
25-42: Excellent Rector configuration setup.The configuration applies comprehensive rule sets and properly enforces strict types, which will help maintain high code quality standards in the test suite.
.vortex/tests/phpunit/tests/Traits/LoggerTrait.php (1)
11-25: Excellent use of debug_backtrace for automatic function naming.The automatic function name detection using
debug_backtraceis a clever solution that reduces boilerplate in test methods. The fallback to 'unknown' provides good error resilience..vortex/tests/phpunit/tests/Traits/Steps/StepDownloadDbTrait.php (1)
17-33: Excellent step implementation with proper assertions.The method properly validates preconditions, performs the operation, and verifies postconditions. The logging provides good visibility into the process flow.
.vortex/tests/bats/_helper.bash (4)
339-339: LGTM: Environment variable logic correctly inverted.The logic inversion from
VORTEX_DEV_VOLUMES_MOUNTEDtoVORTEX_DEV_VOLUMES_SKIP_MOUNTis correctly implemented. The condition!= "1"properly handles the new semantics where the variable indicates when to skip mounting.
348-348: LGTM: Consistent logic inversion applied.The same correct logic inversion pattern is applied here, maintaining consistency across the codebase.
364-364: LGTM: Logic inversion correctly implemented.The condition correctly uses
= "1"to check when volumes should be skipped, which aligns with the new variable semantics.
422-422: LGTM: Final instance of logic inversion correctly applied.All instances of the environment variable logic have been consistently updated to use the new
VORTEX_DEV_VOLUMES_SKIP_MOUNTvariable with proper logic inversion..vortex/tests/phpunit/tests/Traits/ProcessTrait.php (1)
40-42:Details
✅ Verification successful
Review command validation regex for edge cases.
The regex pattern
/[^a-zA-Z0-9_\-\.\/]/may be too restrictive and could reject valid commands. Consider these edge cases:
- Commands with spaces in paths (though arrays handle this)
- Commands with extensions like
.shor.py- System commands that might contain other valid characters
🏁 Script executed:
#!/bin/bash # Description: Check for common system commands that might be used in tests # Test: Find commands that would fail the current validation # Check common system commands echo "Testing common commands against the regex pattern:" commands=("bash" "docker-compose" "git" "npm" "composer" "ahoy" "drush" "php" "node" "./script.sh" "/usr/bin/php") for cmd in "${commands[@]}"; do if echo "$cmd" | grep -qE '[^a-zA-Z0-9_\-\.\/]'; then echo "WOULD FAIL: $cmd" else echo "WOULD PASS: $cmd" fi doneLength of output: 1606
🏁 Script executed:
#!/bin/bash # Locate the ProcessTrait.php file and display its content around processRun file=$(fd ProcessTrait.php -t f | head -n 1) echo "Found file at: $file" sed -n '1,200p' "$file"Length of output: 152
🏁 Script executed:
#!/bin/bash # Include hidden directories to locate the file file=$(fd -H --type f ProcessTrait.php | head -n 1) if [ -z "$file" ]; then echo "ProcessTrait.php not found; please verify the path manually." exit 1 fi echo "Found file at: $file" # Show the snippet around the validation logic sed -n '1,200p' "$file" | sed -n '35,55p'Length of output: 1083
Command validation regex is appropriately restrictive
The pattern
/[^a-zA-Z0-9_\-\.\/]/correctly allows hyphens, underscores, dots, and slashes—covering commands likedocker-compose,./script.sh,/usr/bin/php, and files with extensions (.sh,.py). It deliberately excludes whitespace and shell‐special characters to prevent injection; any arguments (including spaces) should be passed via the$argumentsarray. No change required here unless you specifically need to permit additional characters (e.g. Windows drive letters or colons), in which case you can extend the character class..vortex/tests/phpunit/tests/Traits/ArrayTrait.php (1)
54-58:⚠️ Potential issueFix logic error in empty array handling.
The logic for empty sub-arrays in
assertArrayNotContainsArrayis incorrect. An empty array is indeed a subset of any array (including non-empty arrays), so this should not fail when the sub-array is empty and the main array is non-empty.The current logic fails when checking if a non-empty array does NOT contain an empty sub-array, but mathematically, an empty set is a subset of every set. The assertion should only fail if we're checking that an empty array does NOT contain an empty array (which would be false since ∅ ⊆ ∅).
public function assertArrayNotContainsArray(array $array, array $subArray): void { - // Empty subArray against empty array should pass (both are empty) - // Empty subArray against non-empty array should fail (empty set is subset) if (empty($subArray)) { - if (!empty($array)) { - $this->fail('Empty sub-array is a subset of any non-empty array.'); - } return; }Likely an incorrect or invalid review comment.
.vortex/tests/phpunit/tests/Traits/Steps/StepBuildTrait.php (2)
78-78: LGTM: Consistent volume mounting logic.The
volumesMounted()method call aligns with the environment variable changes made in the BATS helper file, maintaining consistency across the testing framework.
31-31:Details
❓ Verification inconclusive
Consider making demo database URL configurable.
The hardcoded reference to
static::VORTEX_INSTALL_DEMO_DB_TESTassumes this constant exists in the using class. Consider making this more explicit or configurable.
🏁 Script executed:
#!/bin/bash # Description: Check if VORTEX_INSTALL_DEMO_DB_TEST constant is defined in related classes # Test: Search for the constant definition ast-grep --pattern 'const VORTEX_INSTALL_DEMO_DB_TEST = $_'Length of output: 61
🏁 Script executed:
#!/bin/bash # Search for any occurrences of VORTEX_INSTALL_DEMO_DB_TEST across the repository rg -n "VORTEX_INSTALL_DEMO_DB_TEST"Length of output: 35
Verify that
VORTEX_INSTALL_DEMO_DB_TESTis defined or make the demo DB URL configurable.I couldn’t find any definition of the
VORTEX_INSTALL_DEMO_DB_TESTconstant in the repository. Referencing an undefined constant will cause a runtime error. Please confirm that this constant is declared in the consuming class or update the trait to accept a configurable URL (for example, via an environment variable or constructor parameter).• File:
.vortex/tests/phpunit/tests/Traits/Steps/StepBuildTrait.php, line 31'VORTEX_DB_DOWNLOAD_URL' => static::VORTEX_INSTALL_DEMO_DB_TEST,.vortex/tests/phpunit/tests/Traits/Steps/StepPrepareSutTrait.php (1)
31-33: Ensure.ideadirectory exists before dumping a file.
File::dump()will throw if the target directory hierarchy is absent.
Either create the directory or rely on the helper to do it explicitly.- File::dump(static::locationsSut() . DIRECTORY_SEPARATOR . '.idea/idea_file.txt'); + $ideaDir = static::locationsSut() . DIRECTORY_SEPARATOR . '.idea'; + if (!is_dir($ideaDir)) { + mkdir($ideaDir, 0777, true); + } + File::dump($ideaDir . DIRECTORY_SEPARATOR . 'idea_file.txt');.vortex/tests/phpunit/tests/Traits/GitTrait.php (1)
166-170: Quote tag annotation safely.Passing the message with embedded quotes risks shell-escaping issues on some systems.
Preferescapeshellarg()or drop the quotes:- $repo->createTag($name, ['--message="Annotation for tag ' . $name . '"', '-a']); + $repo->createTag($name, ['-a', '-m', sprintf('Annotation for tag %s', $name)]);.vortex/tests/phpunit/tests/Traits/AssertFilesTrait.php (1)
35-58: Verify contradictory README assertions
assertCommonFilesPresent()(line 14) expectsREADME.mdto exist, whileassertCommonFilesAbsent()(line 53) explicitly asserts that it does not exist.
If both assertion groups can be triggered within the same test suite (e.g. before/after a cleanup step) this is fine, otherwise one of the expectations is wrong.Please double-check the intended lifecycle or add a clarifying comment.
|
@coderabbitai pause |
✅ Actions performedReviews paused. |
AlexSkrypnyk
force-pushed
the
feature/1560-replace-bats-phpunit
branch
from
e0169e1 to
e64bad7
Compare
AlexSkrypnyk
force-pushed
the
feature/1560-replace-bats-phpunit
branch
from
6ab3c97 to
daca9ef
Compare
AlexSkrypnyk
force-pushed
the
feature/1560-replace-bats-phpunit
branch
from
3457f87 to
45f13a2
Compare
AlexSkrypnyk
force-pushed
the
feature/1560-replace-bats-phpunit
branch
from
45f13a2 to
5e757ca
Compare
AlexSkrypnyk
force-pushed
the
feature/1560-replace-bats-phpunit
branch
from
5e757ca to
907b8d5
Compare
AlexSkrypnyk
force-pushed
the
feature/1560-replace-bats-phpunit
branch
from
449b29b to
97174e2
Compare
AlexSkrypnyk
changed the title
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
1 similar comment
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 19
♻️ Duplicate comments (1)
.vortex/tests/phpunit/Traits/GitTrait.php (1)
352-361: Same PHPDoc mis-ordering ingitAssertFilesNotTracked()Mirror the fix applied to
gitAssertFilesTracked()for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (3)
.vortex/tests/composer.lockis excluded by!**/*.lock.vortex/tests/package-lock.jsonis excluded by!**/package-lock.json.vortex/tests/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (22)
.circleci/config.yml(1 hunks).github/workflows/vortex-test-common.yml(3 hunks).vortex/tests/bats/_helper.bash(4 hunks).vortex/tests/bats/_helper.deployment.bash(1 hunks).vortex/tests/bats/_helper.workflow.bash(2 hunks).vortex/tests/bats/workflow.smoke.bats(1 hunks).vortex/tests/composer.json(1 hunks).vortex/tests/phpcs.xml(1 hunks).vortex/tests/phpstan.neon(1 hunks).vortex/tests/phpunit.xml(1 hunks).vortex/tests/phpunit/Functional/FunctionalTestCase.php(1 hunks).vortex/tests/phpunit/Functional/WorkflowTest.php(1 hunks).vortex/tests/phpunit/Traits/AssertFilesTrait.php(1 hunks).vortex/tests/phpunit/Traits/GitTrait.php(1 hunks).vortex/tests/phpunit/Traits/LoggerTrait.php(1 hunks).vortex/tests/phpunit/Traits/Steps/StepBuildTrait.php(1 hunks).vortex/tests/phpunit/Traits/Steps/StepDownloadDbTrait.php(1 hunks).vortex/tests/phpunit/Traits/Steps/StepPrepareSutTrait.php(1 hunks).vortex/tests/phpunit/Traits/Steps/StepTestBddAllTrait.php(1 hunks).vortex/tests/phpunit/Traits/Steps/StepTestBddTrait.php(1 hunks).vortex/tests/rector.php(1 hunks).vortex/tests/test.workflow.sh(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (7)
.vortex/tests/phpunit/Traits/Steps/StepTestBddTrait.php (3)
.vortex/installer/src/Utils/File.php (1)
File(9-46).vortex/tests/phpunit/Traits/LoggerTrait.php (3)
logStepStart(16-21)logSubstep(30-32)logStepFinish(23-28).vortex/tests/phpunit/Functional/FunctionalTestCase.php (2)
syncToHost(71-78)processRunInContainer(89-101)
.vortex/tests/phpunit/Traits/Steps/StepTestBddAllTrait.php (1)
.vortex/tests/phpunit/Traits/LoggerTrait.php (2)
logStepStart(16-21)logStepFinish(23-28)
.vortex/tests/phpunit/Traits/Steps/StepDownloadDbTrait.php (2)
.vortex/installer/src/Utils/File.php (1)
File(9-46).vortex/tests/phpunit/Traits/LoggerTrait.php (3)
logStepStart(16-21)logSubstep(30-32)logStepFinish(23-28)
.vortex/tests/phpunit/Traits/Steps/StepBuildTrait.php (3)
.vortex/tests/phpunit/Traits/LoggerTrait.php (3)
logStepStart(16-21)logSubstep(30-32)logStepFinish(23-28).vortex/tests/phpunit/Functional/FunctionalTestCase.php (1)
syncToHost(71-78).vortex/tests/phpunit/Traits/AssertFilesTrait.php (1)
assertCommonFilesPresent(19-38)
.vortex/tests/phpunit/Traits/Steps/StepPrepareSutTrait.php (5)
.vortex/installer/src/Utils/File.php (1)
File(9-46).vortex/tests/phpunit/Traits/LoggerTrait.php (3)
logStepStart(16-21)logSubstep(30-32)logStepFinish(23-28).vortex/tests/phpunit/Traits/AssertFilesTrait.php (2)
assertCommonFilesAbsent(40-77)assertCommonFilesPresent(19-38).vortex/tests/phpunit/Traits/GitTrait.php (2)
gitAssertIsRepository(178-199)gitCommitAll(269-273).vortex/tests/phpunit/Functional/FunctionalTestCase.php (1)
volumesMounted(103-105)
.vortex/tests/phpunit/Traits/GitTrait.php (2)
.vortex/installer/src/Utils/File.php (1)
File(9-46).vortex/tests/phpunit/Functional/FunctionalTestCase.php (1)
assertFilesExist(107-111)
.vortex/tests/phpunit/Functional/FunctionalTestCase.php (4)
.vortex/installer/src/Utils/File.php (1)
File(9-46).vortex/installer/tests/Unit/UnitTestCase.php (1)
UnitTestCase(20-61).vortex/tests/phpunit/Functional/WorkflowTest.php (2)
setUp(14-23)tearDown(25-29).vortex/tests/phpunit/Traits/LoggerTrait.php (1)
logSubstep(30-32)
🪛 GitHub Check: codecov/patch
.vortex/tests/bats/_helper.bash
[warning] 1348-1348: .vortex/tests/bats/_helper.bash#L1348
Added line #L1348 was not covered by tests
🔇 Additional comments (24)
.github/workflows/vortex-test-common.yml (1)
31-31: LGTM! Environment variable naming improvement.The change from
VORTEX_DEV_VOLUMES_MOUNTED=0toVORTEX_DEV_VOLUMES_SKIP_MOUNT=1makes the intent more explicit and clear. The variable name now directly indicates the action (skip mounting) rather than requiring users to interpret the inverse logic.Also applies to: 112-112, 172-172
.vortex/tests/bats/workflow.smoke.bats (1)
74-74: LGTM! Condition updated to match new environment variable.The condition correctly uses the new
VORTEX_DEV_VOLUMES_SKIP_MOUNTvariable with appropriate default value. The logic maintains the same behavior: copy database files into the container when volume mounting is skipped..vortex/tests/test.workflow.sh (3)
29-29: LGTM! Conditional PHP dependency installation.The conditional installation of PHP dependencies follows the same pattern as the Node dependencies and prevents unnecessary reinstalls.
42-46: LGTM! PHPUnit function follows consistent pattern.The
phpunitfunction implementation follows the same structure as the existingbatsfunction, maintaining consistency in the codebase. The directory navigation and cleanup are handled appropriately.
52-52:Details
✅ Verification successful
Verify PHPUnit test file exists.
The script now runs a specific PHPUnit test file (
phpunit/Functional/WorkflowTest.php) before the BATS tests. Please ensure this test file exists and is properly configured.
🏁 Script executed:
#!/bin/bash # Description: Verify that the PHPUnit test file exists and check its structure # Expected: File should exist and contain valid PHPUnit test structure # Check if the test file exists if [ -f ".vortex/tests/phpunit/Functional/WorkflowTest.php" ]; then echo "✓ WorkflowTest.php exists" echo "File content preview:" head -20 ".vortex/tests/phpunit/Functional/WorkflowTest.php" else echo "✗ WorkflowTest.php not found" fi # Check if phpunit.xml configuration exists if [ -f ".vortex/tests/phpunit.xml" ]; then echo "✓ phpunit.xml configuration exists" else echo "✗ phpunit.xml configuration not found" fi # Check if composer.json has PHPUnit dependencies if [ -f ".vortex/tests/composer.json" ]; then echo "✓ composer.json exists" echo "PHPUnit-related dependencies:" jq '.require | to_entries[] | select(.key | contains("phpunit"))' .vortex/tests/composer.json 2>/dev/null || echo "No PHPUnit dependencies found or jq not available" else echo "✗ composer.json not found" fiLength of output: 968
All PHPUnit test file and configuration are present
- Confirmed that
.vortex/tests/phpunit/Functional/WorkflowTest.phpexists and begins with a valid PHPUnit test class.- Verified that
.vortex/tests/phpunit.xmlis in place.- Checked
composer.jsonunder the tests directory and confirmed PHPUnit is declared as a dependency.No further action required.
.circleci/config.yml (1)
453-453: LGTM! Consistent environment variable update.The environment variable change aligns with the updates in GitHub Actions workflows and test scripts, maintaining consistency across all CI/CD platforms.
.vortex/tests/phpstan.neon (1)
1-24:Details
✅ Verification successful
PHPStan configuration looks well-structured.
The configuration is appropriate for a new testing setup:
- Level 9 provides maximum strictness for new code
- Proper exclusion of vendor directories
- Reasonable ignore rule for test data providers
Consider verifying that the ignore pattern is specific enough to avoid suppressing legitimate type issues:
🏁 Script executed:
#!/bin/bash # Description: Check if there are other array-related type issues that might be inappropriately ignored # Search for potential array type issues in the phpunit directory rg -A 3 -B 3 "array" .vortex/tests/phpunit/ || echo "No phpunit directory found yet" # Check if the ignore pattern is too broad echo "Current ignore pattern covers: 'no value type specified in iterable type array'"Length of output: 12290
Ignore pattern is specific and safe to keep.
No other array‐related type errors are being suppressed by this rule—only messages containing “no value type specified in iterable type array” in phpunit tests.
Configuration is approved as-is..vortex/tests/bats/_helper.deployment.bash (1)
125-130:Details
✅ Verification successful
Environment variable rename improves clarity.
The change from
VORTEX_DEV_VOLUMES_MOUNTEDtoVORTEX_DEV_VOLUMES_SKIP_MOUNTwith inverted logic makes the intention clearer. The variable name now directly indicates what action should be taken.Verify that this environment variable change is consistent across all files:
🏁 Script executed:
#!/bin/bash # Description: Ensure consistent usage of the new environment variable across the codebase # Check for any remaining references to the old variable rg "VORTEX_DEV_VOLUMES_MOUNTED" --type bash # Verify new variable usage patterns rg "VORTEX_DEV_VOLUMES_SKIP_MOUNT" --type bash -A 2 -B 2Length of output: 158
Let’s retry the search without restricting to an unrecognized file type:
🏁 Script executed:
#!/bin/bash # Description: Ensure there are no leftover references to the old var and that the new one is used everywhere # Look for any uses of the old variable rg "VORTEX_DEV_VOLUMES_MOUNTED" # Confirm new variable is referenced in context rg -n "VORTEX_DEV_VOLUMES_SKIP_MOUNT"Length of output: 69
Let’s narrow the search to the
.vortexdirectory where the test helper lives:
🏁 Script executed:
#!/bin/bash # Search specifically in the .vortex folder for both the old and new vars echo "🔍 Searching for old variable:" rg -n "VORTEX_DEV_VOLUMES_MOUNTED" .vortex || echo "— no matches for old var" echo "🔍 Searching for new variable:" rg -n "VORTEX_DEV_VOLUMES_SKIP_MOUNT" .vortex || echo "— no matches for new var"Length of output: 1243
Environment variable rename is consistent across the codebase
Verified that
VORTEX_DEV_VOLUMES_MOUNTEDno longer appears andVORTEX_DEV_VOLUMES_SKIP_MOUNTis referenced in all expected test helpers and functional tests under.vortex/tests/. No further action needed..vortex/tests/bats/_helper.workflow.bash (2)
11-13: Environment variable refactoring is consistent.The change to
VORTEX_DEV_VOLUMES_SKIP_MOUNTwith default value 0 aligns with the refactoring in other files and improves code clarity.
526-538: Improved synchronization and formatting enhances reliability.The addition of
sync_to_hostcall after successful BDD tests ensures that test artifacts are properly synchronized before assertions. The blank lines improve code readability by logically separating operations..vortex/tests/phpcs.xml (1)
1-31: Well-configured PHPCS setup for PHP testing infrastructure.The configuration properly extends Drupal coding standards and includes appropriate exclusions for test files while enforcing strict types. The targeted scope and test-specific rule relaxations are well-thought-out.
.vortex/tests/phpunit.xml (2)
3-3: Verify bootstrap file location.The bootstrap path assumes
vendor/autoload.phpexists relative to this configuration file. Ensure the Composer autoloader is properly installed in the expected location before running tests.
1-31: Excellent PHPUnit 11.x configuration with comprehensive settings.The configuration includes appropriate strict failure settings, detailed error reporting, and correct exclusion of test code from coverage analysis. The JUnit XML logging will be helpful for CI integration.
.vortex/tests/phpunit/Traits/LoggerTrait.php (2)
7-14: Good use of trait composition for logging functionality.The extension of the upstream LoggerTrait provides a clean way to add project-specific logging methods while reusing existing functionality.
16-21: 🛠️ Refactor suggestionAdd bounds checking for debug_backtrace access.
The code accesses
$trace[1]without verifying the backtrace has sufficient frames, which could cause undefined index errors in edge cases.Apply this diff to add proper bounds checking:
public static function logStepStart(?string $message = NULL): void { $trace = debug_backtrace(DEBUG_BACKTRACE_IGNORE_ARGS, 2); - $step = $trace[1]['function'] ?? 'unknown'; + $step = $trace[1]['function'] ?? ($trace[0]['function'] ?? 'unknown'); static::logSection('STEP START | ' . $step, $message, FALSE, 40); }Likely an incorrect or invalid review comment.
.vortex/tests/rector.php (1)
25-64: Comprehensive and well-configured Rector setup.The configuration includes appropriate rule sets for PHP 8.2/8.3, sensible rule exclusions to prevent overly aggressive refactoring, and proper handling of imports. The exclusion of vendor and node_modules directories is correct.
.vortex/tests/phpunit/Traits/Steps/StepTestBddTrait.php (2)
34-35:Details
❓ Verification inconclusive
Verify method dependencies exist in the trait hierarchy.
The code uses
assertDirectoryContainsStringandassertFileExistsmethods that should be available through the DirectoryAssertionsTrait, but it's worth confirming these methods exist and have the expected signatures.Run this script to verify the method availability:
🏁 Script executed:
#!/bin/bash # Search for the assertion methods used in the trait rg -A 5 "assertDirectoryContainsString|assertFileExists" --type phpLength of output: 69
🏁 Script executed:
#!/bin/bash # Locate DirectoryAssertionsTrait definition rg -n "trait DirectoryAssertionsTrait" --glob '*.php' # Search for custom assertion methods rg -n "assertDirectoryContainsString" --glob '*.php' rg -n "assertFileExists" --glob '*.php' # Find the file name if it exists fd --ignore-case DirectoryAssertionsTrait.phpLength of output: 196
Verify availability of assertion methods
Please confirm that the
StepTestBddTrait(or one of its parent classes/traits) actually pulls in the customassertDirectoryContainsStringmethod, and that its signature matches what you’re calling. Note that:
assertDirectoryContainsStringis not a built-in PHPUnit assertion—ensure it’s defined (e.g. in aDirectoryAssertionsTrait) and that the trait is imported.assertFileExistscomes fromPHPUnit\Framework\Assert, so it will be available as long as your test class extends PHPUnit’sTestCase.Locations to check:
.vortex/tests/phpunit/Traits/Steps/StepTestBddTrait.php(lines 34-35)- The definition file for any directory-related assertion trait (e.g.
DirectoryAssertionsTrait.php)
40-41:Details
❓ Verification inconclusive
Verify glob pattern handling in container commands.
The cleanup commands use glob patterns (
*) which may not expand correctly when passed as arguments tormviaprocessRunInContainer. This could result in incomplete cleanup or command failures.Please verify that the glob patterns work correctly in the container environment, or consider using alternative approaches:
🏁 Script executed:
#!/bin/bash # Test glob pattern expansion in container commands docker compose exec cli find /app/.logs/screenshots -type f -name "*" | head -5 docker compose exec cli find /app/.logs/test_results -type f -name "*" | head -5Length of output: 358
🏁 Script executed:
#!/bin/bash # Locate and display implementation of processRunInContainer rg -n "processRunInContainer" -t php || trueLength of output: 43
Verify glob pattern expansion in cleanup commands
I wasn’t able to locate the
processRunInContainerimplementation in this repo, so it’s unclear whether it invokes a shell (and thus expands*) or executesrmdirectly with literal arguments. Please confirm that these two calls actually expand and remove all files under those directories:$this->processRunInContainer('rm', ['-rf', '/app/.logs/screenshots/*']); $this->processRunInContainer('rm', ['-rf', '/app/.logs/test_results/*']);If they don’t, you’ll need to either invoke a shell to enable globbing—for example:
$this->processRunInContainer('bash', ['-lc', 'rm -rf /app/.logs/screenshots/*']);or switch to a
find … -deleteapproach:$this->processRunInContainer('bash', ['-lc', 'find /app/.logs/screenshots -type f -delete']);.vortex/tests/phpunit/Traits/Steps/StepDownloadDbTrait.php (1)
22-37: LGTM - Well-structured database download step.The method implementation is clean and follows good practices:
- Proper logging of step lifecycle
- Verification before and after operations
- Clear environment variable override for testing
- Appropriate assertions for success validation
.vortex/tests/phpunit/Functional/WorkflowTest.php (2)
39-51: Excellent idempotence test design.The test method effectively validates that the build process is idempotent by:
- Running build and verifying Git tracking
- Running BDD tests
- Re-building and verifying consistency
- Re-running BDD tests
This comprehensive approach ensures the workflow produces consistent results across multiple executions.
12-12:Details
✅ Verification successful
Verify FunctionalTestCase base class exists and provides required methods.
The class extends
FunctionalTestCasewhich should provide foundational test utilities. Ensure this base class exists and provides the expected methods likestepPrepareSut(),processRun(), etc.
🏁 Script executed:
#!/bin/bash # Search for the FunctionalTestCase base class rg -A 10 "class FunctionalTestCase" --type phpLength of output: 48
Let’s locate the actual definition file for
FunctionalTestCase. Running a broader search:
🏁 Script executed:
#!/bin/bash # 1. Look for any files whose name hints at FunctionalTestCase find . -type f -iname "*functional*testcase*.php" # 2. Search for the class declaration explicitly rg -n "class FunctionalTestCase" --type php # 3. Search all mentions in case it’s prefixed or namespaced rg -n "FunctionalTestCase" --type phpLength of output: 253
Let’s inspect both candidate files to find the
FunctionalTestCasedefinition and check for the required methods:
🏁 Script executed:
#!/bin/bash # Dump the first 200 lines of each to locate the class and its methods sed -n '1,200p' ./.vortex/tests/phpunit/Functional/FunctionalTestCase.php echo "----" sed -n '1,200p' ./.vortex/installer/tests/Functional/FunctionalTestCase.phpLength of output: 8189
FunctionalTestCase base class and methods confirmed
The
FunctionalTestCaseused byWorkflowTest.phpis located at
.vortex/tests/phpunit/Functional/FunctionalTestCase.phpand includes the following traits that provide the expected utilities:
- ProcessTrait →
processRun(),processStreamOutput,processTearDown(), etc.- StepPrepareSutTrait →
stepPrepareSut(), plus other step methods- StepBuildTrait, StepDownloadDbTrait, StepTestBddAllTrait, StepTestBddTrait
- AssertArrayTrait, AssertFilesTrait, EnvTrait, GitTrait, LocationsTrait, LoggerTrait
No missing dependencies or undefined methods were detected.
.vortex/tests/bats/_helper.bash (2)
339-339: LGTM - Correct logic inversion for environment variable refactoring.The logic inversion is correctly implemented:
- Changed from checking if volumes are NOT mounted (
!= "1")- To checking if mount should be skipped (
!= "1"inverted to= "1")- This maintains the same functional behavior while using clearer semantics
Also applies to: 348-348
339-339:Details
✅ Verification successful
Verify environment variable usage consistency across the codebase.
The refactoring from
VORTEX_DEV_VOLUMES_MOUNTEDtoVORTEX_DEV_VOLUMES_SKIP_MOUNTwith inverted logic looks correct, but we should ensure this change is applied consistently throughout the entire codebase.Also applies to: 348-348, 364-364, 422-422
🏁 Script executed:
#!/bin/bash # Search for any remaining usage of the old environment variable rg "VORTEX_DEV_VOLUMES_MOUNTED" --type bash --type yml --type yaml # Verify consistent usage of the new environment variable rg "VORTEX_DEV_VOLUMES_SKIP_MOUNT" --type bash --type yml --type yaml -A 2 -B 2Length of output: 204
To correct the file-type error and search all files, let’s rerun without
--type bashand include context for the new var:
🏁 Script executed:
#!/bin/bash # Search entire repo for any remaining usage of the old environment variable rg "VORTEX_DEV_VOLUMES_MOUNTED" # Search entire repo for the new environment variable, with 2 lines of context rg "VORTEX_DEV_VOLUMES_SKIP_MOUNT" -C 2Length of output: 71
Let’s re-run the search including hidden and ignored files (like the
.vortextest directory) to be sure we didn’t miss any occurrences:
🏁 Script executed:
#!/bin/bash # Search everything (including hidden/ignored) for the old var rg -uu "VORTEX_DEV_VOLUMES_MOUNTED" # Search everything (including hidden/ignored) for the new var, with context rg -uu -C2 "VORTEX_DEV_VOLUMES_SKIP_MOUNT"Length of output: 5386
Environment variable refactoring verified – all occurrences updated and consistent
I’ve confirmed there are no remaining references to
VORTEX_DEV_VOLUMES_MOUNTEDand the newVORTEX_DEV_VOLUMES_SKIP_MOUNTis used correctly everywhere:• .github/workflows/vortex-test-common.yml
• .vortex/tests/phpunit/Functional/FunctionalTestCase.php
• .vortex/tests/bats/{workflow.smoke.bats,_helper.workflow.bash,_helper.bash,_helper.deployment.bash}
• .circleci/config.ymlLogic inversion matches the intended “skip mount” semantics.
.vortex/tests/phpunit/Traits/AssertFilesTrait.php (1)
58-63: Conflicting expectations forREADME.mdpresence
assertCommonFilesAbsent()unconditionally asserts thatREADME.mddoes not exist,
yetassertCommonFilesPresent()(line 27) asserts the file does exist.If the same test suite can invoke both helpers in different contexts, document the intended
scenarios; otherwise one of the assertions is likely incorrect.
| if (!$process->isSuccessful()) { | ||
| $this->logSubstep('Re-running all BDD tests after random failure'); | ||
| $this->processRun('ahoy', ['test-bdd']); | ||
| $this->assertProcessSuccessful(); | ||
| } |
There was a problem hiding this comment.
Fix the retry logic to properly handle failure cases.
The retry logic has a critical flaw: it doesn't check if the retry attempt was successful before proceeding. This could lead to false positives where the test continues even after both attempts fail.
Apply this diff to fix the retry logic:
if (!$process->isSuccessful()) {
$this->logSubstep('Re-running all BDD tests after random failure');
- $this->processRun('ahoy', ['test-bdd']);
- $this->assertProcessSuccessful();
+ $process = $this->processRun('ahoy', ['test-bdd']);
+ $this->assertProcessSuccessful();
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In .vortex/tests/phpunit/Traits/Steps/StepTestBddTrait.php around lines 25 to
29, the retry logic does not verify if the retry attempt succeeded, potentially
allowing failures to go unnoticed. Modify the code to check the success of the
retry by capturing the result of the second processRun call and then asserting
its success with assertProcessSuccessful. This ensures the test only passes if
the retry attempt is successful.
|
|
||
| parent::setUp(); | ||
|
|
||
| chdir(static::$sut); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider adding error handling for directory change operation.
The chdir(static::$sut) operation could fail if the SUT directory doesn't exist or isn't accessible. While the smoke test verifies the directory exists, adding explicit error handling would improve robustness.
-chdir(static::$sut);
+if (!chdir(static::$sut)) {
+ $this->fail('Failed to change to SUT directory: ' . static::$sut);
+}🤖 Prompt for AI Agents
In .vortex/tests/phpunit/Functional/WorkflowTest.php at line 19, the
chdir(static::$sut) call lacks error handling. Modify the code to check the
return value of chdir, and if it returns false, throw an exception or trigger an
error with a clear message indicating the directory change failed. This will
ensure the test fails explicitly if the directory is not accessible.
| * @param string $path | ||
| * Path to the repository directory. | ||
| * @param string $format | ||
| * Format of commits. | ||
| * | ||
| * @return array<string> | ||
| * Array of commit hashes, sorted from the earliest to the latest commit. | ||
| * | ||
| * @throws \Exception | ||
| */ | ||
| protected function gitGetAllCommits(string $path, string $format = '%s'): array { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Doc-block says “commit hashes” while default format returns subjects
gitGetAllCommits() defaults to --format=%s, i.e. subject lines.
Either change the default to %H (hash) or update the PHPDoc so expectations are aligned.
- * @param string $format
- * Format of commits.
+ * @param string $format
+ * Format passed to `git log --format`. Defaults to `%s` (commit subject);
+ * use `%H` for full hashes.
...
- * Array of commit hashes, sorted from the earliest to the latest commit.
+ * Array of commit identifiers in the chosen format, ordered earliest->latest.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| * @param string $path | |
| * Path to the repository directory. | |
| * @param string $format | |
| * Format of commits. | |
| * | |
| * @return array<string> | |
| * Array of commit hashes, sorted from the earliest to the latest commit. | |
| * | |
| * @throws \Exception | |
| */ | |
| protected function gitGetAllCommits(string $path, string $format = '%s'): array { | |
| * @param string $path | |
| * Path to the repository directory. | |
| * @param string $format | |
| * Format passed to `git log --format`. Defaults to `%s` (commit subject); | |
| * use `%H` for full hashes. | |
| * | |
| * @return array<string> | |
| * Array of commit identifiers in the chosen format, ordered earliest->latest. | |
| * | |
| * @throws \Exception | |
| */ | |
| protected function gitGetAllCommits(string $path, string $format = '%s'): array { |
🤖 Prompt for AI Agents
In .vortex/tests/phpunit/Traits/GitTrait.php around lines 90 to 100, the PHPDoc
states that the method returns commit hashes, but the default format string '%s'
returns commit subjects instead. To fix this, update the default format
parameter in the method signature from '%s' to '%H' so it returns commit hashes
as documented, ensuring the PHPDoc and method behavior are consistent.
| protected function createDevelopmentDrupalSettings(string $webroot = 'web'): void { | ||
| Assert::assertFileDoesNotExist($webroot . '/sites/default/settings.local.php'); | ||
| Assert::assertFileDoesNotExist($webroot . '/sites/default/services.local.yml'); | ||
| Assert::assertFileExists($webroot . '/sites/default/default.settings.local.php'); | ||
| Assert::assertFileExists($webroot . '/sites/default/default.services.local.yml'); | ||
|
|
||
| copy($webroot . '/sites/default/default.settings.local.php', $webroot . '/sites/default/settings.local.php'); | ||
| copy($webroot . '/sites/default/default.services.local.yml', $webroot . '/sites/default/services.local.yml'); | ||
|
|
||
| Assert::assertFileExists($webroot . '/sites/default/settings.local.php'); | ||
| Assert::assertFileExists($webroot . '/sites/default/services.local.yml'); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Missing error handling for copy() and unlink()
If filesystem permissions are restrictive these operations can fail silently.
Wrapping them with assertions or throwing exceptions helps pinpoint issues during CI runs.
Example:
if (!copy(...)) {
throw new \RuntimeException('Failed to copy default.settings.local.php');
}🤖 Prompt for AI Agents
In .vortex/tests/phpunit/Traits/AssertFilesTrait.php around lines 326 to 337,
the copy() function calls lack error handling, which can cause silent failures
if filesystem permissions are restrictive. To fix this, wrap each copy() call
with a conditional that checks its return value, and throw a RuntimeException
with a clear error message if the copy fails. This will ensure that any failure
is caught and reported during CI runs.
| public function assertFilesTrackedInGit(string $webroot = 'web', bool $skip_commit = FALSE): void { | ||
| $this->createDevelopmentDrupalSettings($webroot); | ||
|
|
||
| if (!$skip_commit) { | ||
| $this->gitCommitAll('.', 'Commit fully built project'); | ||
| } | ||
|
|
||
| // Assert that Drupal Scaffold files were added to the git repository. | ||
| $this->gitAssertFilesTracked($webroot . '/.htaccess'); | ||
| $this->gitAssertFilesTracked($webroot . '/autoload.php'); | ||
| $this->gitAssertFilesTracked($webroot . '/index.php'); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Trait implicitly depends on GitTrait
assertFilesTrackedInGit() calls several git* helpers that are defined in GitTrait,
yet this trait does not use GitTrait;.
Document the coupling in the PHPDoc or add the use statement to avoid surprises
when the trait is mixed into classes without GitTrait.
🤖 Prompt for AI Agents
In .vortex/tests/phpunit/Traits/AssertFilesTrait.php around lines 294 to 304,
the method assertFilesTrackedInGit() uses git* helper methods from GitTrait but
the AssertFilesTrait does not include 'use GitTrait;'. To fix this, either add
'use GitTrait;' inside AssertFilesTrait to explicitly include the dependency or
document this dependency clearly in the PHPDoc block of AssertFilesTrait to
inform users that GitTrait must be used alongside it.
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
.vortex/tests/phpcs.xml (1)
1-32: 🧹 Nitpick (assertive)Well-structured PHPCS configuration with appropriate test file exceptions.
The configuration properly:
- Extends Drupal coding standards as base
- Enforces strict types declarations
- Provides reasonable exceptions for test files (long arrays, missing function comments)
- Includes proper CLI options for output formatting
Consider standardizing the exclude patterns to use a single pattern format:
- <exclude-pattern>*.Test\.php</exclude-pattern> - <exclude-pattern>*.TestCase\.php</exclude-pattern> - <exclude-pattern>*.test</exclude-pattern> + <exclude-pattern>*Test*.php</exclude-pattern> + <exclude-pattern>*.test</exclude-pattern>This would cover both
*.Test.phpand*.TestCase.phppatterns more concisely.
♻️ Duplicate comments (21)
.vortex/tests/phpunit/Traits/Steps/StepTestBddAllTrait.php (1)
12-19: Placeholder implementation still needs actual BDD test execution logic.This method remains a placeholder with only logging calls. The actual BDD test execution logic is missing.
.vortex/tests/phpunit/Traits/LoggerTrait.php (2)
16-21: Add bounds checking for debug_backtrace access.The code accesses
$trace[1]['function']without verifying that$trace[1]exists, which could cause an "Undefined array key" error if the backtrace has fewer than 2 entries.Apply this diff to add proper bounds checking:
public static function logStepStart(?string $message = NULL): void { $trace = debug_backtrace(DEBUG_BACKTRACE_IGNORE_ARGS, 2); - $step = $trace[1]['function'] ?? 'unknown'; + $step = (isset($trace[1]) ? $trace[1]['function'] : null) ?? 'unknown'; static::logSection('STEP START | ' . $step, $message, FALSE, 40); }
23-28: Add bounds checking for debug_backtrace access.Same bounds checking issue as
logStepStart- the code needs to verify$trace[1]exists before accessing it.Apply this diff to add proper bounds checking:
public static function logStepFinish(?string $message = NULL): void { $trace = debug_backtrace(DEBUG_BACKTRACE_IGNORE_ARGS, 2); - $step = $trace[1]['function'] ?? 'unknown'; + $step = (isset($trace[1]) ? $trace[1]['function'] : null) ?? 'unknown'; static::logSection('STEP DONE | ' . $step, $message, FALSE, 40); }.vortex/tests/phpunit/Traits/Steps/StepTestBddTrait.php (1)
25-29: Fix the retry logic to properly handle failure cases.The retry logic doesn't capture the result of the second
processRuncall, which could lead to false positives ifassertProcessSuccessful()doesn't refer to the retry attempt.Apply this diff to fix the retry logic:
if (!$process->isSuccessful()) { $this->logSubstep('Re-running all BDD tests after random failure'); - $this->processRun('ahoy', ['test-bdd']); + $process = $this->processRun('ahoy', ['test-bdd']); $this->assertProcessSuccessful(); }.vortex/tests/phpunit/Traits/Steps/StepDownloadDbTrait.php (1)
20-20: Hardcoded database URL reduces maintainability.The constant uses a hardcoded URL to a specific version (24.11.0) of the test database, which could become outdated or inaccessible over time.
.vortex/tests/phpunit/Functional/WorkflowTest.php (1)
19-19: Add error handling for directory change operation.The
chdir(static::$sut)operation could fail if the SUT directory doesn't exist or isn't accessible. Adding explicit error handling would improve robustness..vortex/tests/phpunit/Traits/Steps/StepBuildTrait.php (1)
24-40: 🛠️ Refactor suggestionStill using positional parameters in
processRun()despite earlier feedbackA previous review already highlighted that relying on positional offsets for
processRun()is brittle and obscures intent. The call on these lines still passes five arguments positionally.
Switching to named-parameter style makes the code self-documenting and shields it from signature changes:- $this->processRun('ahoy', ['build'], ['y'], [ - 'VORTEX_DB_DOWNLOAD_URL' => static::VORTEX_INSTALL_DEMO_DB_TEST, - 'VORTEX_CONTAINER_REGISTRY_USER' => getenv('TEST_VORTEX_CONTAINER_REGISTRY_USER') ?: '', - 'VORTEX_CONTAINER_REGISTRY_PASS' => getenv('TEST_VORTEX_CONTAINER_REGISTRY_PASS') ?: '', - ], 10 * 60); + $this->processRun( + command: 'ahoy', + arguments: ['build'], + inputs: ['y'], + env: [ + 'VORTEX_DB_DOWNLOAD_URL' => static::VORTEX_INSTALL_DEMO_DB_TEST, + 'VORTEX_CONTAINER_REGISTRY_USER' => getenv('TEST_VORTEX_CONTAINER_REGISTRY_USER') ?: '', + 'VORTEX_CONTAINER_REGISTRY_PASS' => getenv('TEST_VORTEX_CONTAINER_REGISTRY_PASS') ?: '', + ], + timeout: 10 * 60, + );.vortex/tests/phpunit/Traits/Steps/StepPrepareSutTrait.php (2)
94-99: 🛠️ Refactor suggestion“Blanket”
##replacement indocker-compose.ymlis still over-aggressiveReplacing every occurrence of
##wipes legitimate comment markers, not just the special placeholders. The earlier review suggested anchoring the match; it’s still unchanged.- File::replaceContent('docker-compose.yml', '##', ''); + // Remove only lines that *start* with the magic marker. + File::replaceContent('docker-compose.yml', '/^\\s*##\\s+/m', '');This preserves normal double-hash comments while stripping the intended markers.
115-118: 🛠️ Refactor suggestionEscaped path in
.ahoy.ymlwill execute a literally back-slashed binaryThe replacement string keeps back-slashes (
\.\/) before dots and slashes, causing the shell to look for a file named\.\/scripts\/vortex\/provision\.sh. Earlier feedback covered this, but the escapes remain:- 'cmd: if [ -f .data/db.sql ]; then docker compose exec cli mkdir -p .data; docker compose cp -L .data/db.sql cli:/app/.data/db.sql; fi; ahoy cli \.\/scripts\/vortex\/provision\.sh', + 'cmd: if [ -f .data/db.sql ]; then docker compose exec cli mkdir -p .data && docker compose cp -L .data/db.sql cli:/app/.data/db.sql; fi; ahoy cli ./scripts/vortex/provision.sh',Besides removing the escapes, chaining the two
docker composecommands with&&avoids continuing when the first one fails..vortex/tests/composer.json (1)
53-54: 🧹 Nitpick (assertive)*
"reset"script is still nix-only – breaks on Windows shellsThe prior review proposed a platform-agnostic deletion strategy. The current
"rm -Rf vendor"will fail on Windows:- "reset": "rm -Rf vendor", + "reset": "php -r \"echo shell_exec(PHP_OS_FAMILY==='Windows' ? 'rmdir /S /Q vendor' : 'rm -rf vendor');\"",This keeps the developer UX identical while working cross-platform.
.vortex/tests/phpunit/Functional/FunctionalTestCase.php (2)
65-68: Shell-injection risk infixtureExportCodebase()remains unresolved
The tar/pipe command still interpolates$dstwithout escaping, allowing a malicious path to break out of the command. This was raised previously and should be fixed withescapeshellarg().
103-105: Prefer strict comparison for env flag
Loose!=comparison can mis-interpret values like'true','yes', etc. A strict identity check keeps the intent unambiguous:
return getenv('VORTEX_DEV_VOLUMES_SKIP_MOUNT') !== '1';.vortex/tests/phpunit/Traits/GitTrait.php (5)
81-85: Add missingstringtype-hint to$pathingitReset()
The parameter is always used as a string elsewhere. The earlier review still applies.
90-100: Doc-block says “commit hashes” but default format returns subjects
Either change the default to%Hor fix the PHPDoc. Comment previously raised.
134-146: Guard against out-of-range indexes ingitGetCommitsRange()
Accessing$commits[$key]without a bounds check will raise notices for short histories. Previously reported.
186-198: Replace shell call ingitAssertIsRepository()with GitPhp API
Usingshell_exec()introduces quoting/portability issues and duplicates functionality already provided byCzProject\GitPhp. Prior feedback still holds.
327-333: PHPDoc parameter descriptions forgitAssertFilesTracked()are swapped
Still mismatched ($filesvs$path). Please align the documentation..vortex/tests/phpunit/Traits/AssertFilesTrait.php (4)
19-31: Hard-coded project name reduces re-usability
Defaulting to'star_wars'breaks consumers scaffolding a differently named project. Earlier suggestion to inject or derive the project name remains unaddressed.
294-301: Trait implicitly depends onGitTraitbut doesn’tuseit
Methods callgit*()helpers that are provided elsewhere. Document the dependency or import the trait.
326-334:copy()return value ignored – potential silent failure
If permissions are restrictive, the copies may fail with no feedback. Add checks and raise exceptions. Same issue was flagged previously.
339-345:unlink()calls lack error handling
Silent failure will leave artefacts in the workspace. Prior feedback still applies.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (3)
.vortex/tests/composer.lockis excluded by!**/*.lock.vortex/tests/package-lock.jsonis excluded by!**/package-lock.json.vortex/tests/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (22)
.circleci/config.yml(1 hunks).github/workflows/vortex-test-common.yml(3 hunks).vortex/tests/bats/_helper.bash(4 hunks).vortex/tests/bats/_helper.deployment.bash(1 hunks).vortex/tests/bats/_helper.workflow.bash(2 hunks).vortex/tests/bats/workflow.smoke.bats(1 hunks).vortex/tests/composer.json(1 hunks).vortex/tests/phpcs.xml(1 hunks).vortex/tests/phpstan.neon(1 hunks).vortex/tests/phpunit.xml(1 hunks).vortex/tests/phpunit/Functional/FunctionalTestCase.php(1 hunks).vortex/tests/phpunit/Functional/WorkflowTest.php(1 hunks).vortex/tests/phpunit/Traits/AssertFilesTrait.php(1 hunks).vortex/tests/phpunit/Traits/GitTrait.php(1 hunks).vortex/tests/phpunit/Traits/LoggerTrait.php(1 hunks).vortex/tests/phpunit/Traits/Steps/StepBuildTrait.php(1 hunks).vortex/tests/phpunit/Traits/Steps/StepDownloadDbTrait.php(1 hunks).vortex/tests/phpunit/Traits/Steps/StepPrepareSutTrait.php(1 hunks).vortex/tests/phpunit/Traits/Steps/StepTestBddAllTrait.php(1 hunks).vortex/tests/phpunit/Traits/Steps/StepTestBddTrait.php(1 hunks).vortex/tests/rector.php(1 hunks).vortex/tests/test.workflow.sh(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (6)
.vortex/tests/phpunit/Traits/Steps/StepTestBddAllTrait.php (1)
.vortex/tests/phpunit/Traits/LoggerTrait.php (2)
logStepStart(16-21)logStepFinish(23-28)
.vortex/tests/phpunit/Traits/Steps/StepTestBddTrait.php (3)
.vortex/installer/src/Utils/File.php (1)
File(9-46).vortex/tests/phpunit/Traits/LoggerTrait.php (3)
logStepStart(16-21)logSubstep(30-32)logStepFinish(23-28).vortex/tests/phpunit/Functional/FunctionalTestCase.php (2)
syncToHost(71-78)processRunInContainer(89-101)
.vortex/tests/phpunit/Traits/Steps/StepPrepareSutTrait.php (5)
.vortex/installer/src/Utils/File.php (1)
File(9-46).vortex/tests/phpunit/Traits/LoggerTrait.php (3)
logStepStart(16-21)logSubstep(30-32)logStepFinish(23-28).vortex/tests/phpunit/Traits/AssertFilesTrait.php (2)
assertCommonFilesAbsent(40-77)assertCommonFilesPresent(19-38).vortex/tests/phpunit/Traits/GitTrait.php (2)
gitAssertIsRepository(178-199)gitCommitAll(269-273).vortex/tests/phpunit/Functional/FunctionalTestCase.php (1)
volumesMounted(103-105)
.vortex/tests/phpunit/Traits/Steps/StepDownloadDbTrait.php (2)
.vortex/installer/src/Utils/File.php (1)
File(9-46).vortex/tests/phpunit/Traits/LoggerTrait.php (3)
logStepStart(16-21)logSubstep(30-32)logStepFinish(23-28)
.vortex/tests/phpunit/Functional/FunctionalTestCase.php (4)
.vortex/installer/src/Utils/File.php (1)
File(9-46).vortex/installer/tests/Unit/UnitTestCase.php (1)
UnitTestCase(20-61).vortex/tests/phpunit/Functional/WorkflowTest.php (2)
setUp(14-23)tearDown(25-29).vortex/tests/phpunit/Traits/LoggerTrait.php (1)
logSubstep(30-32)
.vortex/tests/phpunit/Traits/GitTrait.php (2)
.vortex/installer/src/Utils/File.php (1)
File(9-46).vortex/tests/phpunit/Functional/FunctionalTestCase.php (1)
assertFilesExist(107-111)
🪛 GitHub Check: codecov/patch
.vortex/tests/bats/_helper.bash
[warning] 1348-1348: .vortex/tests/bats/_helper.bash#L1348
Added line #L1348 was not covered by tests
🔇 Additional comments (22)
.github/workflows/vortex-test-common.yml (3)
31-31: LGTM: Environment variable renamed with clearer semantics.The change from
VORTEX_DEV_VOLUMES_MOUNTED=0toVORTEX_DEV_VOLUMES_SKIP_MOUNT=1improves clarity by using positive rather than negative logic.
112-112: LGTM: Consistent environment variable update.The environment variable change is consistent with the pattern applied across all jobs.
172-172: LGTM: Consistent environment variable update.The environment variable change maintains consistency across all job definitions.
.vortex/tests/bats/workflow.smoke.bats (1)
74-74: LGTM: Conditional logic correctly updated for environment variable change.The logic is properly inverted to match the semantic change from
VORTEX_DEV_VOLUMES_MOUNTEDtoVORTEX_DEV_VOLUMES_SKIP_MOUNT. The default value handling with${VORTEX_DEV_VOLUMES_SKIP_MOUNT:-0}is appropriate..vortex/tests/test.workflow.sh (3)
29-29: LGTM: Added PHP dependency installation.The addition of PHP dependency installation using Composer follows the same pattern as the existing Node dependency installation and properly supports the new PHPUnit tests.
42-46: LGTM: Well-structured PHPUnit function.The
phpunitfunction follows the same pattern as the existingbatsfunction, properly changing directory and executing PHPUnit from the test directory.
52-52:Details
✅ Verification successful
Verify PHPUnit test file exists.
The script references
"${TEST_DIR}"/phpunit/Functional/WorkflowTest.php. Please ensure this test file exists in the repository.
🏁 Script executed:
#!/bin/bash # Description: Verify that the PHPUnit test file exists # Expected: The test file should be present fd --type f "WorkflowTest.php" --exec ls -la {}Length of output: 49
🏁 Script executed:
#!/bin/bash # Locate the test.workflow.sh script script_path=$(fd -e sh test.workflow.sh) echo "Script path: $script_path" # Show where TEST_DIR is defined in that script echo "---- TEST_DIR definition ----" grep -R "TEST_DIR" -n "$script_path" || echo "TEST_DIR not defined in $script_path" # List contents of phpunit/Functional relative to the script location functional_dir=$(dirname "$script_path")/phpunit/Functional echo "---- Listing $functional_dir ----" ls -la "$functional_dir" || echo "$functional_dir not found" # Find any PHP files with 'Workflow' and 'Test' in their name echo "---- Searching for *workflow*test*.php across repo ----" find . -type f -iname "*workflow*test*.php" || echo "No matching files found" # Search for class definitions referencing WorkflowTest echo "---- Grepping for class definitions of WorkflowTest ----" rg "class\s+WorkflowTest" -g "*.php" -n || echo "No class definitions of WorkflowTest found"Length of output: 3316
Confirmed: WorkflowTest.php exists
Verified that the file
.vortex/tests/phpunit/Functional/WorkflowTest.phpis present in the repository, so the reference in.vortex/tests/test.workflow.shis correct. No changes required..circleci/config.yml (1)
453-453: LGTM: Consistent environment variable update.The environment variable change from
VORTEX_DEV_VOLUMES_MOUNTED=0toVORTEX_DEV_VOLUMES_SKIP_MOUNT=1is consistent with the coordinated refactoring across all CI configurations..vortex/tests/phpstan.neon (1)
1-24: Well-configured PHPStan setup with appropriate strictness level.The configuration follows PHPStan best practices:
- Level 9 provides maximum strictness for code quality
- Proper path inclusion and exclusion settings
- Reasonable ignore rule for test file array types where docblocks may be missing
.vortex/tests/bats/_helper.deployment.bash (1)
125-125:Details
✅ Verification successful
Verify consistent environment variable usage across codebase.
The environment variable has been renamed from
VORTEX_DEV_VOLUMES_MOUNTEDtoVORTEX_DEV_VOLUMES_SKIP_MOUNTwith inverted logic. While the AI summary mentions coordinated changes, this needs verification to ensure no references to the old variable remain.
🏁 Script executed:
#!/bin/bash # Description: Check for any remaining references to the old environment variable # Expected: No references to VORTEX_DEV_VOLUMES_MOUNTED should remain echo "Searching for old environment variable references..." rg "VORTEX_DEV_VOLUMES_MOUNTED" --type sh --type bash --type yml --type yaml echo -e "\nSearching for new environment variable usage..." rg "VORTEX_DEV_VOLUMES_SKIP_MOUNT" --type sh --type bash --type yml --type yaml -A 2 -B 2Length of output: 450
🏁 Script executed:
#!/bin/bash # Re-run search without relying on type definitions to ensure coverage echo "Searching for old environment variable references..." rg -w "VORTEX_DEV_VOLUMES_MOUNTED" echo -e "\nSearching for new environment variable usage..." rg -w "VORTEX_DEV_VOLUMES_SKIP_MOUNT" -C 2Length of output: 303
🏁 Script executed:
#!/bin/bash # Include hidden files and directories in the search (e.g., .vortex/) echo "Searching for old environment variable references in all files (including hidden)..." rg --hidden -w "VORTEX_DEV_VOLUMES_MOUNTED" -n echo -e "\nSearching for new environment variable references in all files (including hidden)..." rg --hidden -w "VORTEX_DEV_VOLUMES_SKIP_MOUNT" -n -C2Length of output: 6022
Environment variable rename and logic inversion verified
A repository-wide search (including hidden files) confirms that
VORTEX_DEV_VOLUMES_MOUNTEDno longer appears and every reference has been updated toVORTEX_DEV_VOLUMES_SKIP_MOUNT. No further changes are needed..vortex/tests/bats/_helper.workflow.bash (2)
526-535: LGTM! Improved synchronization handling.The explicit
sync_to_hostcalls after successful BDD test runs and before assertions ensure proper file synchronization. The added blank lines improve readability and logical separation of the test steps.
11-13:Details
✅ Verification successful
Verify environment variable consistency across the codebase.
The environment variable name changed from
VORTEX_DEV_VOLUMES_MOUNTED(default 1) toVORTEX_DEV_VOLUMES_SKIP_MOUNT(default 0), which flips the default behavior. According to the AI summary, this change aligns with similar updates across multiple test helper scripts and CI configurations.
🏁 Script executed:
#!/bin/bash # Verify that all references to the old environment variable have been updated echo "Searching for old environment variable references..." rg "VORTEX_DEV_VOLUMES_MOUNTED" --type bash --type yaml --type yml echo -e "\nSearching for new environment variable usage..." rg "VORTEX_DEV_VOLUMES_SKIP_MOUNT" --type bash --type yaml --type ymlLength of output: 420
The previous ripgrep invocation didn’t match
.bashor.batsfiles. Let’s broaden the search to all shell and YAML extensions:
🏁 Script executed:
#!/bin/bash echo "Searching for old variable references across shell and YAML files..." rg "VORTEX_DEV_VOLUMES_MOUNTED" -g '*.{sh,bash,bats,yml,yaml}' echo -e "\nSearching for new variable usage..." rg "VORTEX_DEV_VOLUMES_SKIP_MOUNT" -g '*.{sh,bash,bats,yml,yaml}'Length of output: 362
Let’s search without any ignore rules (including hidden/.vortex):
🏁 Script executed:
#!/bin/bash echo "Deep search for old variable (including hidden files)…" rg -n VORTEX_DEV_VOLUMES_MOUNTED --no-ignore --hidden echo -e "\nDeep search for new variable (including hidden files)…" rg -n VORTEX_DEV_VOLUMES_SKIP_MOUNT --no-ignore --hiddenLength of output: 1730
Environment variable rename verified and consistent across the codebase
- No occurrences of
VORTEX_DEV_VOLUMES_MOUNTEDremain.- All references in test helpers, deployment scripts, CI workflows, and PHP tests now use
VORTEX_DEV_VOLUMES_SKIP_MOUNTwith the intended default flip.- No further changes are required.
.vortex/tests/phpunit.xml (1)
1-31: Excellent PHPUnit configuration setup!The configuration is comprehensive and well-structured:
- Proper bootstrap and execution order settings
- Strict failure modes for risky tests and warnings enhance test reliability
- Detailed error reporting with all warning/error types displayed
- Source configuration appropriately excludes test directory from coverage
- JUnit XML logging enables CI integration
This provides a solid foundation for the new PHPUnit-based functional testing suite.
.vortex/tests/phpunit/Traits/LoggerTrait.php (1)
30-32: LGTM! Simple and effective substep logging.The
logSubstepmethod provides clean, consistent formatting for substep messages with appropriate indentation and line breaks..vortex/tests/phpunit/Traits/Steps/StepTestBddTrait.php (3)
19-24: LGTM! Well-structured step initiation.Good logging practices with step start logging and clear substep messaging. The initial BDD test execution is properly structured.
31-36: LGTM! Proper verification and synchronization.The host synchronization and assertions properly verify that BDD tests generated the expected artifacts (screenshots and test results XML). The assertion messages are descriptive and helpful for debugging.
37-44: LGTM! Comprehensive cleanup implementation.The cleanup logic properly removes both host and container artifacts, ensuring a clean state for subsequent tests. The step finish logging maintains consistency with the logging pattern.
.vortex/tests/phpunit/Traits/Steps/StepDownloadDbTrait.php (1)
22-37: Well-structured step method with proper logging and assertions.The method follows a clear pattern:
- Logs step start/finish for visibility
- Cleans up existing files before operation
- Uses assertions to verify state changes
- Provides clear substep logging for debugging
The implementation correctly uses environment variable override to point the
ahoy download-dbcommand to the test database URL..vortex/tests/rector.php (1)
25-64: Comprehensive and well-configured Rector setup.The configuration includes:
- Modern PHP version targets (8.2, 8.3)
- Essential rule sets for code quality and style
- PHPUnit 10.0 compatibility rules
- Sensible exclusions for problematic rules
- Proper directory exclusions (vendor, node_modules)
The selective rule skipping shows thoughtful consideration of which automated refactoring rules to avoid, preventing overly aggressive changes that might reduce code readability.
.vortex/tests/phpunit/Functional/WorkflowTest.php (2)
34-37: Effective smoke test for environment validation.The smoke test properly validates:
- SUT directory existence
- Current working directory matches SUT
This provides a good baseline check before running more complex tests.
39-51: Well-designed idempotence test.The test effectively validates that:
- Initial build produces trackable files in Git
- BDD tests pass on first run
- Rebuild produces identical results (idempotence)
- BDD tests continue to pass after rebuild
The use of
skip_commit: TRUEon the second Git assertion prevents unnecessary commits while still validating file tracking..vortex/tests/bats/_helper.bash (1)
1339-1339: Consistent environment variable migration with proper logic inversion.The migration from
VORTEX_DEV_VOLUMES_MOUNTEDtoVORTEX_DEV_VOLUMES_SKIP_MOUNTis correctly implemented across all functions:
sync_to_host()andsync_to_container()skip operations whenSKIP_MOUNT=1fix_host_dependencies()andprocess_ahoyyml()apply modifications whenSKIP_MOUNT=1The logic inversion is properly handled with appropriate default values (
:-0) and condition operators.Also applies to: 1348-1348, 1364-1364, 1422-1422
github-project-automation
bot
moved this from To do for current release
to Release queue
in Vortex
#1560
Summary by CodeRabbit
New Features
Chores
Documentation