Added tests for docs generator. #383
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis update introduces and configures PHPUnit-based unit testing to the project. It adds a PHPUnit configuration file, new development dependencies, and a dedicated autoload section for unit tests. The Changes
Sequence Diagram(s)sequenceDiagram
participant Developer
participant CLI
participant PHPUnit
participant Codebase
Developer->>CLI: Run "ahoy test-unit" or "ahoy test-unit-coverage"
CLI->>PHPUnit: Execute PHPUnit (with/without coverage)
PHPUnit->>Codebase: Load tests from tests/phpunit/src/
Codebase-->>PHPUnit: Execute test cases
PHPUnit-->>CLI: Output results (with/without coverage report)
CLI-->>Developer: Display test summary and reports
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (13)
🪧 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 (
|
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 (9)
.ahoy.yml(1 hunks).gitignore(1 hunks)composer.json(2 hunks)docs.php(4 hunks)phpcs.xml(1 hunks)phpstan.neon(2 hunks)phpunit.xml(1 hunks)tests/phpunit/src/DocsTest.php(1 hunks)tests/phpunit/src/UnitTestCase.php(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
docs.php (1)
tests/behat/bootstrap/FeatureContext.php (1)
FeatureContext(46-93)
🔇 Additional comments (21)
phpstan.neon (4)
14-14: Good addition of PHPUnit test directory to PHPStan analysisIncluding the unit test directory in static analysis ensures test code is also held to high code quality standards.
23-24: Improved Behat path referenceUsing a relative path for the Behat binary improves consistency and portability across different environments.
25-27: Proper configuration of PHPUnit dependencies for analysisAdding the PHPUnit and helpers packages to scan directories enables PHPStan to better understand the code and reduces false positives in analysis.
29-36: Well-documented error suppression for test filesThe error suppression is:
- Properly scoped to only test files
- Narrowly targeted to a specific error type
- Well-documented with clear reasoning
This is the right approach for handling the common issue of iterable types in test data providers.
.ahoy.yml (2)
135-142: Well-implemented unit test commandsThe new test commands properly integrate PHPUnit into the project workflow. The
test-unitcommand runs basic tests whiletest-unit-coverageenables code coverage reporting via pcov.
148-148: Updated description for consistencyGood update to the description for test-bdd-coverage, making it clear that this command runs tests with coverage, consistent with the new test-unit-coverage command.
composer.json (2)
31-38: Well-structured autoloading for testsThe autoload-dev section is properly configured to support the test infrastructure. Including docs.php in the files section ensures it's available during testing.
48-51: Appropriate test dependenciesThe required dev dependencies are specifically chosen to support the unit testing infrastructure. PHPUnit v11 is the latest version and alexskrypnyk/phpunit-helpers provides useful testing utilities.
docs.php (8)
17-17: Improved CLI interface with path optionGood addition of the
--pathoption to allow specifying a custom base directory, which enhances the script's flexibility and reusability.
22-26: Improved modularity with conditional main executionThe script now only executes the main function when run directly, not when included. This is a good practice that makes the script more testable and reusable as a library.
28-81: Well-structured main function extractionExtracting the main functionality into a separate function improves testability and organization. The function properly handles options and has appropriate code coverage annotations.
100-100: Base path parameter added for better flexibilityThe
extract_infofunction now accepts a base path parameter, allowing for more flexible file resolution. This aligns well with the new--pathCLI option.
207-209: Enhanced robustness in comment parsingEarly return for empty comments improves robustness and prevents potential issues with null or empty values.
310-310: Consistent base path parameter across functionsThe
render_infofunction also accepts a base path parameter now, maintaining consistency with other functions and supporting the--pathCLI option.
317-320: Improved error handling with full pathsError messages now include full paths which makes debugging easier, especially when using custom base paths.
327-331: Appropriate code coverage exclusionsThe code that checks for example file existence is properly excluded from code coverage since it's difficult to test and primarily for runtime validation.
tests/phpunit/src/DocsTest.php (2)
23-35: Typo in data-provider name makes the intent unclearBoth the
#[DataProvider]attribute (line 23) and the corresponding static method (line 35) are spelleddataProverParseMethodComment(“Prover” instead of “Provider”).
While this still works because the names match, the misspelling is confusing for future readers and for IDE code-completion / navigation.-#[DataProvider('dataProverParseMethodComment')] +#[DataProvider('dataProviderParseMethodComment')] ... -public static function dataProverParseMethodComment(): array { +public static function dataProviderParseMethodComment(): array {[ suggest_nitpick ]
253-256: Verify desired behaviour for consecutive capital lettersThe test case for
HTTPRequestexpects the resulth_t_t_p_request, i.e. every capital letter is split by an underscore.
Most “camel-to-snake” helpers convert an acronym to a single word (http_request). Please double-check that the production function really intends the more verbose behaviour and that other call-sites rely on it; otherwise the test may cement an un-idiomatic conversion rule.
[ request_verification ].gitignore (1)
5-5: Approve addition of PHPUnit cache directory to.gitignore.
Ignoring/.phpunit.cacheprevents PHPUnit’s cache files from being committed, keeping the repo clean.phpcs.xml (1)
25-25: Approve inclusion of PHPUnit test source directory in PHPCS configuration.
Adding<file>../tests/phpunit/src</file>aligns the CodeSniffer rules with the new PHPUnit test suite so that test code is style-checked.tests/phpunit/src/UnitTestCase.php (1)
1-14: Approve the new base test case class.
The class correctly enables strict types, declares theDrevOps\BehatSteps\Testsnamespace, and extends the upstream helper, providing a solid foundation for writing unit tests.
phpunit.xml
Outdated
| <?xml version="1.0" encoding="UTF-8"?> | ||
| <phpunit xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="https://schema.phpunit.de/11.4/phpunit.xsd" | ||
| bootstrap="vendor/autoload.php" | ||
| cacheDirectory=".phpunit.cache" | ||
| executionOrder="depends,defects" | ||
| requireCoverageMetadata="true" | ||
| beStrictAboutCoverageMetadata="false" | ||
| beStrictAboutOutputDuringTests="true" | ||
| failOnRisky="true" | ||
| failOnWarning="true" | ||
| displayDetailsOnTestsThatTriggerWarnings="true" | ||
| displayDetailsOnTestsThatTriggerErrors="true" | ||
| displayDetailsOnTestsThatTriggerNotices="true" | ||
| displayDetailsOnTestsThatTriggerDeprecations="true" | ||
| displayDetailsOnPhpunitDeprecations="true"> | ||
| <php> | ||
| <env name="SCRIPT_RUN_SKIP" value="1"/> | ||
| </php> | ||
| <testsuites> | ||
| <testsuite name="default"> | ||
| <directory>tests/phpunit/src/</directory> | ||
| </testsuite> | ||
| </testsuites> | ||
| <source restrictNotices="true" | ||
| restrictWarnings="true" | ||
| ignoreIndirectDeprecations="true"> | ||
| <include> | ||
| <file>docs.php</file> | ||
| </include> | ||
| <exclude> | ||
| <directory>tests/phpunit/src</directory> | ||
| </exclude> | ||
| </source> | ||
| <coverage pathCoverage="false" | ||
| ignoreDeprecatedCodeUnits="true" | ||
| disableCodeCoverageIgnore="false"> | ||
| <report> | ||
| <html outputDirectory=".logs/phpunit/.coverage-html" lowUpperBound="50" highLowerBound="90"/> | ||
| <cobertura outputFile=".logs/phpunit/cobertura.xml"/> | ||
| </report> | ||
| </coverage> | ||
| <logging> | ||
| <junit outputFile=".logs/phpunit/junit.xml"/> | ||
| </logging> | ||
| </phpunit> |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Well-structured PHPUnit configuration
The PHPUnit configuration is comprehensive and follows best practices:
- Proper bootstrap configuration
- Appropriate caching setup
- Test suite correctly targets the new unit tests
- Coverage reporting is well configured with both HTML and Cobertura XML
- JUnit XML logging for CI integration
One suggestion regarding the coverage configuration:
Consider adding a comment explaining the purpose of the SCRIPT_RUN_SKIP environment variable since its usage might not be immediately clear to other developers.
| if (isset($info['MissingTrait'])) { | ||
| $src_file_path = $base_path . DIRECTORY_SEPARATOR . 'src/MissingTrait.php'; | ||
| @unlink($src_file_path); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Minor: remove unused $methods variable to silence IDE warnings
Inside the foreach ($info as $trait => $methods) loop $methods is never used.
Replacing it with the throw-away variable _ (or omitting it with foreach (array_keys($info) as $trait)) avoids “unused variable” inspections.
-foreach ($info as $trait => $methods) {
+foreach (array_keys($info) as $trait) {There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (3)
tests/phpunit/src/DocsTest.php (3)
410-412: Suppressing errors with@mkdir()may hide real problems
(See the same remark in the previous review.)Using
@discards permission and race-condition failures that would otherwise give an immediate, helpful stack-trace. Swap it with an assertion (or remove the@) so the test fails loudly if the directory cannot be created.
414-423: Variable$methodsis never used inside the loopThe value part of the
foreach ($info as $trait => $methods)isn’t referenced, so most IDEs will mark$methodsas unused. Switching toforeach (array_keys($info) as $trait)(or_) silences the warning and conveys intent more clearly.
427-428: 🛠️ Refactor suggestionAvoid silencing potential failures with
@unlink()If
@unlink()fails (e.g. file already removed or permission denied) the test proceeds in an inconsistent state and might produce misleading assertions later.
Remove the@operator and optionally wrap the call withself::assertTrue(unlink($src_file_path))to surface the real error.- @unlink($src_file_path); + self::assertTrue( + !file_exists($src_file_path) || unlink($src_file_path), + "Failed to delete temporary file $src_file_path" + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Lite
📒 Files selected for processing (6)
.ahoy.yml(1 hunks).circleci/config.yml(1 hunks).circleci/provision.sh(1 hunks)README.md(2 hunks)composer.json(2 hunks)tests/phpunit/src/DocsTest.php(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/phpunit/src/DocsTest.php (2)
tests/phpunit/src/UnitTestCase.php (1)
UnitTestCase(12-14)docs.php (6)
parse_method_comment(206-297)camel_to_snake(462-479)array_to_markdown_table(524-536)render_info(310-382)validate(393-449)replace_content(493-511)
🔇 Additional comments (2)
composer.json (1)
31-35: PSR-4 dev autoload and dev dependencies correctly added
The newautoload-devblock andrequire-devsection properly configure PHPUnit and test helpers for theDrevOps\BehatSteps\Tests\namespace. Great setup for your unit testing framework.Also applies to: 45-48
.ahoy.yml (1)
152-153: Usage text refined for BDD coverage command
Thetest-bdd-coverageusage has been updated to clarify it runs BDD tests with coverage. This aligns with the naming of the new unit test commands.
| name: Run Unit tests with coverage | ||
| command: ahoy test-unit-coverage || [ "${CI_TEST_IGNORE_FAILURE:-0}" -eq 1 ] | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add timeouts to new test steps for consistency
The new “Run Unit tests with coverage” and “Run BDD tests with coverage” steps mirror the existing build step but lack a no_output_timeout. To prevent CI hangs on long-running tests, consider adding a no_output_timeout (e.g., no_output_timeout: 30m) to both steps.
Also applies to: 38-40
.circleci/provision.sh
Outdated
| php -r ' | ||
| $base = json_decode(file_get_contents("/app/composer.json"), true); | ||
| $override = json_decode(file_get_contents("/app/build/composer.json"), true); | ||
| unset($base["require-dev"]); | ||
| $base["autoload-dev"] = $base["autoload"]; | ||
| unset($base["autoload"]); | ||
| echo json_encode(array_replace_recursive($base, $override), JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES); | ||
| ' > "/app/build/composer2.json" && mv -f "/app/build/composer2.json" "/app/build/composer.json" |
There was a problem hiding this comment.
Inconsistent comment vs. code for JSON encoding flags
The inline comment advises not to add JSON_UNESCAPED_SLASHES, yet the json_encode call includes it. This discrepancy risks breaking the intended sed-based path replacements.
Apply this diff to align code with the comment:
-echo json_encode(array_replace_recursive($base, $override), JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES);
+echo json_encode(array_replace_recursive($base, $override), JSON_PRETTY_PRINT);📝 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.
| php -r ' | |
| $base = json_decode(file_get_contents("/app/composer.json"), true); | |
| $override = json_decode(file_get_contents("/app/build/composer.json"), true); | |
| unset($base["require-dev"]); | |
| $base["autoload-dev"] = $base["autoload"]; | |
| unset($base["autoload"]); | |
| echo json_encode(array_replace_recursive($base, $override), JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES); | |
| ' > "/app/build/composer2.json" && mv -f "/app/build/composer2.json" "/app/build/composer.json" | |
| php -r ' | |
| $base = json_decode(file_get_contents("/app/composer.json"), true); | |
| $override = json_decode(file_get_contents("/app/build/composer.json"), true); | |
| unset($base["require-dev"]); | |
| $base["autoload-dev"] = $base["autoload"]; | |
| unset($base["autoload"]); | |
| echo json_encode(array_replace_recursive($base, $override), JSON_PRETTY_PRINT); | |
| ' > "/app/build/composer2.json" && mv -f "/app/build/composer2.json" "/app/build/composer.json" |
.ahoy.yml
Outdated
| test-unit: | ||
| usage: Run Unit tests. | ||
| cmd: | | ||
| [ ! -d vendor ] && composer install --prefer-dist --no-progress --no-suggest --no-interaction | ||
| ahoy cli "./vendor/bin/phpunit -c phpunit.xml --no-coverage $@" | ||
|
|
There was a problem hiding this comment.
Ensure PHPUnit executable check in test-unit command
Using the presence of the entire vendor directory to skip composer install can lead to cases where vendor exists but vendor/bin/phpunit is missing (e.g., after a --no-dev install).
Apply this diff:
- [ ! -d vendor ] && composer install --prefer-dist --no-progress --no-suggest --no-interaction
+ [ ! -f vendor/bin/phpunit ] && composer install --prefer-dist --no-progress --no-suggest --no-interaction
.ahoy.yml
Outdated
| test-unit-coverage: | ||
| usage: Run Unit tests with coverage. | ||
| cmd: | | ||
| [ ! -d vendor ] && composer install --prefer-dist --no-progress --no-suggest --no-interaction | ||
| ahoy cli "php -d pcov.directory=. ./vendor/bin/phpunit -c phpunit.xml $@" | ||
|
|
There was a problem hiding this comment.
Enable PCOV when generating coverage in test-unit-coverage
The current command sets pcov.directory but doesn’t enable PCOV, so coverage reports may not be generated.
Apply this diff:
- ahoy cli "php -d pcov.directory=. ./vendor/bin/phpunit -c phpunit.xml $@"
+ ahoy cli "php -dpcov.enabled=1 -d pcov.directory=. ./vendor/bin/phpunit -c phpunit.xml $@"📝 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.
| test-unit-coverage: | |
| usage: Run Unit tests with coverage. | |
| cmd: | | |
| [ ! -d vendor ] && composer install --prefer-dist --no-progress --no-suggest --no-interaction | |
| ahoy cli "php -d pcov.directory=. ./vendor/bin/phpunit -c phpunit.xml $@" | |
| test-unit-coverage: | |
| usage: Run Unit tests with coverage. | |
| cmd: | | |
| [ ! -d vendor ] && composer install --prefer-dist --no-progress --no-suggest --no-interaction | |
| ahoy cli "php -dpcov.enabled=1 -d pcov.directory=. ./vendor/bin/phpunit -c phpunit.xml $@" |
| protected function setUp(): void { | ||
| parent::setUp(); | ||
|
|
||
| require_once __DIR__ . '/../../../docs.php'; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Load the target code once via PHPUnit bootstrap instead of per-test require_once
Including docs.php in setUp() means the file is loaded before every single test method, incurring unnecessary I/O and making it harder to share the bootstrap logic with other future tests.
Consider moving the include to PHPUnit’s bootstrap (see phpunit.xml) or Composer’s autoload-dev so it is executed only once.
- protected function setUp(): void {
- parent::setUp();
-
- require_once __DIR__ . '/../../../docs.php';
- }
+ // setUp() can stay empty once docs.php is required from phpunit.xml:
+ protected function setUp(): void {
+ parent::setUp();
+ }📝 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.
| protected function setUp(): void { | |
| parent::setUp(); | |
| require_once __DIR__ . '/../../../docs.php'; | |
| } | |
| // setUp() can stay empty once docs.php is required from phpunit.xml: | |
| protected function setUp(): void { | |
| parent::setUp(); | |
| } |
90f7e91 to
c8534c3
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (8)
.circleci/config.yml (1)
35-40: 🛠️ Refactor suggestionAdd timeouts to new test steps for consistency
The new "Run Unit tests with coverage" and "Run BDD tests with coverage" steps mirror the existing build step but lack a
no_output_timeout. To prevent CI hangs on long-running tests, consider adding ano_output_timeout(e.g.,no_output_timeout: 30m) to both steps.- run: name: Run Unit tests with coverage command: ahoy test-unit-coverage || [ "${CI_TEST_IGNORE_FAILURE:-0}" -eq 1 ] + no_output_timeout: 30m - run: name: Run BDD tests with coverage command: ahoy test-bdd-coverage || [ "${CI_TEST_IGNORE_FAILURE:-0}" -eq 1 ] + no_output_timeout: 30mREADME.md (1)
136-148: Inconsistent heading levels between test sectionsThe “Unit tests” section uses a fourth-level heading (
####), while “Behat tests” uses a third-level heading (###). For consistency and better hierarchy, align both sections to the same heading level (e.g.,###)..ahoy.yml (2)
139-144:⚠️ Potential issueEnable PCOV in coverage command
Coverage may not be generated since PCOV isn’t enabled. Modify to:
- ahoy cli "php -d pcov.directory=. ./vendor/bin/phpunit -c phpunit.xml $@" + ahoy cli "php -dpcov.enabled=1 -d pcov.directory=. ./vendor/bin/phpunit -c phpunit.xml $@"
133-138:⚠️ Potential issueUse executable check instead of directory presence
Checking only for the existence of
vendorcan skipcomposer installwhenvendor/bin/phpunitis missing. Update the condition:- [ ! -d vendor ] && composer install --prefer-dist --no-progress --no-suggest --no-interaction + [ ! -f vendor/bin/phpunit ] && composer install --prefer-dist --no-progress --no-suggest --no-interactiontests/phpunit/src/DocsTest.php (4)
32-34: Rename typo in data-provider attribute for consistency
dataProverParseMethodCommentlooks like a misspelling of “Provider” and was flagged previously.
Please rename both the method and the attribute reference todataProviderParseMethodCommentto improve readability.-#[DataProvider('dataProverParseMethodComment')] +#[DataProvider('dataProviderParseMethodComment')] -public static function dataProverParseMethodComment(): array { +public static function dataProviderParseMethodComment(): array {
26-30: Loaddocs.phponce via PHPUnit bootstrap instead of per-test includeRequiring the target file in every
setUp()incurs unnecessary I/O.
Move the include tophpunit.xml’s<bootstrap>or rely on Composer autoload.
410-411: Avoid silencing errors with@mkdir()The error-control operator hides real problems (e.g. permission issues) and can cause confusing failures later in the test.
Use an assertion or let the exception surface:-@mkdir($trait_dir, 0777, TRUE); -@mkdir($features_dir, 0777, TRUE); +self::assertTrue(is_dir($trait_dir) || mkdir($trait_dir, 0777, true)); +self::assertTrue(is_dir($features_dir) || mkdir($features_dir, 0777, true));
414-414: Remove unused$methodsvariable to silence IDE/static-analysis warningsThe loop value is never referenced; replace it with a throw-away variable:
-foreach ($info as $trait => $methods) { +foreach (array_keys($info) as $trait) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Lite
📒 Files selected for processing (11)
.ahoy.yml(3 hunks).circleci/config.yml(1 hunks).circleci/provision.sh(1 hunks)README.md(2 hunks)composer.json(2 hunks)phpcs.xml(1 hunks)phpstan.neon(1 hunks)rector.php(1 hunks)tests/behat/fixtures/d10/.gitignore(1 hunks)tests/behat/fixtures/d10/composer.json(3 hunks)tests/phpunit/src/DocsTest.php(1 hunks)
🔇 Additional comments (18)
rector.php (1)
32-32: Hardcoded Drupal root improves configuration simplicityThe change to use a hardcoded path (
$drupalRoot = 'build/web') rather than dynamic detection simplifies the Rector configuration and removes unnecessary dependencies. This aligns well with the project's move toward streamlined testing infrastructure.tests/behat/fixtures/d10/.gitignore (1)
1-3: Appropriate Git ignore patterns for Composer-managed projectThe added ignore patterns are standard for Composer projects and will correctly exclude generated files from version control.
phpcs.xml (2)
23-26: PHPCS file paths correctly include new test directoryThe updated paths properly include the new PHPUnit tests directory and remove relative parent directory prefixes, making paths more consistent and maintainable.
29-30: Correctly updated CircleCI config exclusion patternsExclusion patterns for CircleCI configuration files now use the correct relative paths without parent directory prefixes.
phpstan.neon (4)
12-15: Approve static analysis paths updateIncluding
tests/phpunit/srcand removing relative parent prefixes ensures PHPStan analyzes all relevant code.
20-21: Approve updated excludePathsExcluding
tests/behat/bootstrap/BehatCliContext.phpprevents unwanted context files from being analyzed.
25-28: Approve scanDirectories expansionAdding
vendor/phpunit/phpunitandvendor/alexskrypnyk/phpunit-helpersallows PHPStan to inspect development dependencies for better type coverage.
29-37: Approve ignoreErrors configurationSuppressing missing iterable type annotation errors in test files is a scoped and maintainable approach.
tests/behat/fixtures/d10/composer.json (4)
5-5: Approve project type for fixtureUsing
"type": "project"accurately reflects that this is a standalone Drupal site fixture.
23-23: Approve retainedwebflo/drupal-finderdependencyIncluding
webflo/drupal-finderis necessary for locating the Drupal root; its presence here aligns with fixture requirements.
26-26: Approve minimalrequire-devinclusionKeeping only
drupal/core-devas a development dependency ensures essential tooling for the fixture while minimizing bloat.
45-46: Approve fixtureconfigadjustmentsSetting
"process-timeout": 0and"sort-packages": trueprevents composer timeouts and ensures deterministic installs in CI..ahoy.yml (4)
33-36: Approve DB command chaining adjustmentsUsing
&&ensures commands execute sequentially on macOS and improves readability.
114-118: Approve lint command simplificationRunning linters directly without copying config files reduces complexity and keeps the pipeline DRY.
122-123: Approve lint-fix command cleanupRemoving redundant copy steps and running PHPCBF directly streamlines code style corrections.
150-151: Verify PCOV directory path for BDD coverageEnsure
-dpcov.directory=/app/srcpoints to the correct source directory when running Behat coverage, as fixtures execute in/app/build.composer.json (2)
50-54: 👍 Good addition ofautoload-devfor test namespaceMapping
DrevOps\BehatSteps\Tests\totests/phpunit/src/enables IDE discovery and Composer autoloading of the new test classes.
No action required.
39-42:✅ Verification successful
Potential dependency conflict between PHPUnit 11 and Prophecy bridge
phpspec/prophecy-phpunit2.x is only compatible with PHPUnit ≤ 9.x.
Requiringphpunit/phpunit": "^11"will cause Composer to enter a version-solving conflict or silently install an incompatible major of Prophecy (none exists yet).Consider one of the following:
- "phpspec/prophecy-phpunit": "^2.3", - "phpunit/phpunit": "^11", + "phpspec/prophecy-phpunit": "^3.0", # when released for PHPUnit ≥10 + "phpunit/phpunit": "^10 || ^11" # or pin to ^10 for nowPlease run
composer update --dry-runto verify the constraint graph before merging.
🏁 Script executed:
#!/bin/bash set -e # Fetch Prophecy bridge package metadata from Packagist and list PHPUnit constraints for v2.x releases curl -s 'https://repo.packagist.org/p2/phpspec/prophecy-phpunit.json' | \ jq -r '.packages["phpspec/prophecy-phpunit"][] | select(.version | test("^2\\.")) | "\(.version) => PHPUnit constraint: \(.require["phpunit/phpunit"] // "none")"'Length of output: 251
🏁 Script executed:
#!/bin/bash set -e URL='https://repo.packagist.org/p2/phpspec/prophecy-phpunit.json' echo "====== Root keys ======" curl -s "$URL" | jq 'keys' echo "====== packages keys ======" curl -s "$URL" | jq '.packages | keys' echo "====== Fields in first version entry ======" curl -s "$URL" | jq '.packages["phpspec/prophecy-phpunit"][0] | keys' echo "====== require and require-dev for v2.x versions ======" curl -s "$URL" | jq -r ' .packages["phpspec/prophecy-phpunit"][] | select(.version | test("^2\\.")) | "\(.version) => require: \((.require // {}) | keys | join(",")), require-dev: \((.["require-dev"] // {}) | keys | join(","))"'Length of output: 1336
🏁 Script executed:
#!/bin/bash set -e URL="https://repo.packagist.org/p2/phpspec/prophecy-phpunit.json" echo "v2.x versions and their phpunit/phpunit constraints:" curl -s "$URL" | jq -r ' .packages["phpspec/prophecy-phpunit"][] | select(.version | test("^2\\.")) | "\(.version)\t\(.require["phpunit/phpunit"] // "<none>")"'Length of output: 411
🏁 Script executed:
#!/bin/bash set -e URL="https://repo.packagist.org/p2/phpspec/prophecy-phpunit.json" echo "ALL VERSIONS:" curl -s "$URL" | jq -r '.packages["phpspec/prophecy-phpunit"][] | .version' echo echo "VERSION => REQUIRE:" curl -s "$URL" | jq -r ' .packages["phpspec/prophecy-phpunit"][] | "\(.version) => \((.require // {}) | to_entries | map("\(.key)=\(.value)") | join(", "))"'Length of output: 992
phpspec/prophecy-phpunit ^2.3 supports PHPUnit 11 – no conflict
The constraint
"phpspec/prophecy-phpunit": "^2.3"will install v2.3.0, which declares support forphpunit/phpunit^9.1 || ^10.1 || ^11.0. Requiring"phpunit/phpunit": "^11"therefore satisfies both constraints without issue.No changes required in composer.json (lines 39–42).
| $package_filtered["autoload"] = $package["autoload"]; | ||
| $package_filtered["autoload-dev"]["psr-4"]["DrevOps\\BehatSteps\\"] = "../src/"; | ||
|
|
||
| echo json_encode(array_replace_recursive($package_filtered, $fixture), JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES); |
There was a problem hiding this comment.
Inconsistent JSON encoding flags
The inline comment warns against JSON_UNESCAPED_SLASHES, yet the json_encode call includes it. This mismatch can break the subsequent sed path replacement. Remove JSON_UNESCAPED_SLASHES from the flags.
| php -r ' | ||
| $package = json_decode(file_get_contents("/app/composer.json"), true); | ||
| $fixture = json_decode(file_get_contents("/app/build/composer.json"), true); | ||
|
|
||
| // Cherry-pick the required properties from the base composer.json. | ||
| $package_filtered["require-dev"] = $package["require"]; | ||
|
|
||
| // Deps required to run Behat tests. | ||
| $package_filtered["require-dev"] = array_merge($package_filtered["require-dev"], array_filter($package["require-dev"], function ($ver, $name) { | ||
| return in_array($name, [ | ||
| "drevops/behat-phpserver", | ||
| "drevops/behat-screenshot", | ||
| "dvdoug/behat-code-coverage", | ||
| ]); | ||
| }, ARRAY_FILTER_USE_BOTH)); | ||
| unset($package_filtered["require-dev"]["php"]); | ||
|
|
||
| $package_filtered["autoload"] = $package["autoload"]; | ||
| $package_filtered["autoload-dev"]["psr-4"]["DrevOps\\BehatSteps\\"] = "../src/"; | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider modularizing merge logic
Embedding multi-step PHP for composer-json merging directly in the shell script reduces readability and maintainability. Extract this block into a standalone PHP script (e.g., scripts/merge-fixture-composer.php) and invoke it from here.
| $actual = camel_to_snake($input, $separator); | ||
| $this->assertEquals($expected, $actual); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Nitpick: prefer assertSame when comparing strings
Using assertSame() (strict) instead of assertEquals() avoids accidental type juggling and gives slightly clearer failure messages for string tests.
- $this->assertEquals($expected, $actual);
+ $this->assertSame($expected, $actual);📝 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.
| $actual = camel_to_snake($input, $separator); | |
| $this->assertEquals($expected, $actual); | |
| } | |
| $actual = camel_to_snake($input, $separator); | |
| $this->assertSame($expected, $actual); | |
| } |
c8534c3 to
14b6ef2
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (5)
.circleci/config.yml (1)
35-40: Add timeouts to new test steps
The new “Run Unit tests with coverage” and “Run BDD tests with coverage” steps mirror existing steps but lack ano_output_timeout. To prevent CI hangs on long-running tests, addno_output_timeout: 30mto both..circleci/provision.sh (1)
28-49: 🧹 Nitpick (assertive)Selective merge logic is solid, but modularizing it would improve maintainability.
The inline PHP block effectively cherry-picks dependencies and autoload entries, but embedding multi-step logic in the shell script reduces readability. Consider extracting this into a standalone PHP script (e.g.,scripts/merge-composer.php) and invoking it here for clarity..ahoy.yml (1)
138-141: Enable PCOV for coverage reports.
The coverage command setspcov.directorybut does not enable PCOV. Without-dpcov.enabled=1, coverage may not be generated. Apply:- ahoy cli "php -d pcov.directory=. ./vendor/bin/phpunit -c phpunit.xml $@" + ahoy cli "php -dpcov.enabled=1 -d pcov.directory=. ./vendor/bin/phpunit -c phpunit.xml $@"tests/phpunit/src/DocsTest.php (2)
26-30: Loaddocs.phponce via PHPUnit bootstrap instead of every testIncluding the script inside
setUp()re-executes the file before every test method, wasting I/O and hiding fatal errors behind dozens of stack traces.
Move the include tophpunit.xml(<phpunit bootstrap="docs.php">) or Composer autoload so it is executed exactly once.(This was noted in an earlier review)
409-414: 🛠️ Refactor suggestionCheck
mkdir()return value to surface permission problemsIf the directory cannot be created (e.g. read-only FS in CI) the test continues and fails later in a non-obvious place.
Capture the boolean result and assert success:-mkdir($trait_dir, 0777, true); -mkdir($features_dir, 0777, true); +self::assertTrue( + is_dir($trait_dir) || mkdir($trait_dir, 0777, true), + "Failed to create $trait_dir" +); +self::assertTrue( + is_dir($features_dir) || mkdir($features_dir, 0777, true), + "Failed to create $features_dir" +);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Lite
📒 Files selected for processing (12)
.ahoy.yml(4 hunks).circleci/config.yml(1 hunks).circleci/provision.sh(1 hunks)README.md(2 hunks)composer.json(2 hunks)phpcs.xml(1 hunks)phpstan.neon(1 hunks)phpunit.xml(1 hunks)rector.php(1 hunks)tests/behat/fixtures/d10/.gitignore(1 hunks)tests/behat/fixtures/d10/composer.json(3 hunks)tests/phpunit/src/DocsTest.php(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/phpunit/src/DocsTest.php (2)
tests/phpunit/src/UnitTestCase.php (1)
UnitTestCase(12-14)docs.php (6)
parse_method_comment(206-297)camel_to_snake(462-479)array_to_markdown_table(524-536)render_info(310-382)validate(393-449)replace_content(493-511)
🔇 Additional comments (25)
phpcs.xml (2)
23-26: Include PHPUnit tests in PHPCS scanning
Addingtests/phpunit/srcto the<file>paths ensures your new unit tests are analyzed by PHPCS, and removing the../prefix makes the paths relative to the repo root—this aligns with other configuration updates.
29-30: Exclude updated CircleCI config patterns
Removing the../prefix from the<exclude-pattern>entries correctly matches the current file locations for CircleCI configs.tests/behat/fixtures/d10/.gitignore (1)
1-3: Review.gitignoreentries for the fixture
Ignoringcomposer.lockandvendor/is appropriate to keep the fixture directory clean. However, confirm whether.gitattributesshould indeed be ignored—sometimes it's beneficial to keep it versioned to enforce consistent file handling.README.md (1)
142-146: Good use of language identifier
Includingbashon the unit test code block ensures proper syntax highlighting and readability..circleci/provision.sh (1)
48-48: Ensure JSON_UNESCAPED_SLASHES does not interfere with path adjustments.
You’re usingJSON_UNESCAPED_SLASHES; confirm that the subsequentsedsubstitution still matches correctly, or adjust the regex if needed.phpstan.neon (4)
12-15: Paths updated for new test sources.
Includingtests/phpunit/srcanddocs.phpinpathsaligns with the added unit tests and code under analysis. This ensures PHPStan will analyze the new test sources and documentation utilities.
20-21: Exclude generated Behat context.
Ignoringtests/behat/bootstrap/BehatCliContext.phpreduces noise from automatically generated context classes.
25-28: Register PHPStan extension directories.
Addingvendor/phpunit/phpunitandvendor/alexskrypnyk/phpunit-helpersunderscanDirectoriesis correct for discovering PHPUnit extensions and custom helpers.
29-37: Ignore type annotation errors in test code.
Suppressing “no value type specified in iterable type” errors intests/phpunit/*acknowledges that test methods often omit docblock parameter types. This keeps analysis focused on production code.phpunit.xml (5)
1-15: Review PHPUnit baseline settings.
The configuration enables strict failure modes, caching, and execution ordering, aligning with best practices for test reliability and performance.
17-20: Define default test suite.
Pointing thedefaulttestsuite attests/phpunit/src/correctly scopes your unit tests.
21-30: Configure coverage source inclusion/exclusion.
Includingdocs.phpand excluding the test directory ensures coverage metrics focus on production code.
31-38: Coverage report formats set.
HTML and Cobertura reports with defined thresholds facilitate both human inspection and CI integration.
39-41: JUnit logging for CI.
Generating JUnit XML output in.logs/phpunit/junit.xmlintegrates seamlessly with CI pipelines.tests/behat/fixtures/d10/composer.json (5)
5-5: Project type is appropriate for a fixture.
Changing"type"to"project"aligns with how this composer.json functions as a standalone Drupal site.
23-23: Verify necessity ofwebflo/drupal-finder.
Thewebflo/drupal-finderpackage helps locate Drupal roots, but confirm it’s used by your provisioning or test setup. Remove it if it’s not required by your fixture.
26-27: Minimal dev dependency set.
Including onlydrupal/core-devkeeps the fixture lean while satisfying Drupal’s development requirements.
45-47: Configuration keys updated.
Setting"process-timeout": 0and enabling"sort-packages"improves install reliability and consistency.
54-55: Allow PHPStan extension installer plugin.
Granting permission forphpstan/extension-installermatches the static analysis configuration and supports custom extensions..ahoy.yml (5)
9-15: Improve the dependency install check in the build command.
Currently, it checks for the host'svendordirectory before runningcomposer installinside the CLI container. This could skip installation if the host directory exists but is incomplete. Consider verifying a more specific indicator (like a lockfile or the presence of key binaries) to ensure dependencies are installed correctly.
34-37: Fixed macOS DB command chaining.
The updated use ofmdfindand conditional chaining for Sequel Ace improves compatibility on Darwin systems. This reads cleanly and exits gracefully when not supported.
115-118: Simplified lint command.
Removing redundant file-copy steps and running tools directly via the CLI container streamlines the lint workflow.
123-124: Streamlined lint-fix command.
Executing Rector and PHP Code Beautifier directly in the CLI context enhances maintainability.
146-149: Verify PCOV directory for BDD coverage.
Thepcov.directoryfor BDD tests is set to/app/src, but tests are executed in/app/build. Confirm this path points to the correct source location to ensure accurate coverage metrics.composer.json (1)
50-54: Autoload-dev path looks correct — nice one
tests/phpunit/src/is now discoverable underDrevOps\BehatSteps\Tests\which lets IDEs and PHPUnit resolve test classes without extra bootstrap glue.
| @@ -30,8 +29,7 @@ | |||
| use Rector\TypeDeclaration\Rector\StmtsAwareInterface\DeclareStrictTypesRector; | |||
|
|
|||
| return static function (RectorConfig $rectorConfig): void { | |||
| $drupalFinder = new DrupalFinderComposerRuntime(); | |||
| $drupalRoot = $drupalFinder->getDrupalRoot(); | |||
| $drupalRoot = 'build/web'; | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Simplify Drupal root configuration
Switching to a static $drupalRoot = 'build/web'; streamlines the setup and removes the dependency on runtime detection. Please verify that build/web exists in all environments (CI and local) and update any related documentation or scripts accordingly. You may also consider removing the now-unused imports for the Drupal set lists since those sets have been dropped.
🏁 Script executed:
#!/bin/bash
# Check if the build/web directory exists in the repo
echo "Checking for build/web directory:"
if [ -d build/web ]; then
echo "✅ build/web exists"
else
echo "❌ build/web not found"
fi
# Locate drupalRoot assignment(s)
echo -e "\nSearching for drupalRoot assignments:"
rg "drupalRoot" -n rector.php
# Show the top of rector.php to inspect imports and context
echo -e "\nDisplaying first 100 lines of rector.php:"
sed -n '1,100p' rector.php
# Look for imported SetList classes in rector.php
echo -e "\nSearching for SetList imports in rector.php:"
grep -R "use .*SetList" -n rector.php || echo "No SetList imports found"
# Search globally for any SetList references to see if they’re still in use
echo -e "\nGlobal search for SetList references:"
rg "SetList" -n .Length of output: 4314
Verify drupalRoot and clean up unused imports
- The
build/webdirectory isn’t present in the repo — confirm it’s generated or exists in both CI and local environments, and update any documentation or scripts that depend on it. - In rector.php, remove the now-unused Drupal set list imports at the top of the file (lines 15–17):
- use DrupalRector\Set\Drupal10SetList;
- use DrupalRector\Set\Drupal8SetList;
- use DrupalRector\Set\Drupal9SetList;📝 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.
| $drupalRoot = 'build/web'; | |
| - use DrupalRector\Set\Drupal10SetList; | |
| - use DrupalRector\Set\Drupal8SetList; | |
| - use DrupalRector\Set\Drupal9SetList; |
| There are two types of tests in this repository: unit tests and Behat tests. | ||
|
|
||
| #### Unit tests | ||
|
|
||
| Unit tests are run using PHPUnit installed in the root of the repository and | ||
| are independent of the Drupal version. This allows us to use the latest | ||
| features of PHPUnit. | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Normalize heading level for Unit tests
The new test section intro is clear, but using #### Unit tests makes it one level deeper than top-level sections. Change it to ### Unit tests for consistent hierarchy.
| $package = json_decode(file_get_contents("/app/composer.json"), true); | ||
| $fixture = json_decode(file_get_contents("/app/build/composer.json"), true); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add error handling for JSON parsing.
If json_decode fails, $package or $fixture may be null, leading to invalid output. Consider validating the result of json_decode and exiting with a descriptive error if parsing fails.
| test-unit: | ||
| usage: Run Unit tests. | ||
| cmd: ahoy cli "./vendor/bin/phpunit -c phpunit.xml --no-coverage $@" | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Ensure dependencies before running unit tests.
The test-unit command assumes vendor/bin/phpunit is present. For a smoother developer experience, consider adding a pre-check to install dependencies if missing:
[ ! -f vendor/bin/phpunit ] && composer install --prefer-dist --no-progress --no-suggest --no-interaction| "require-dev": { | ||
| "alexskrypnyk/phpunit-helpers": "^0.5.1", | ||
| "cweagans/composer-patches": "^1.7", | ||
| "dantleech/gherkin-lint": "^0.2.3", | ||
| "dealerdirect/phpcodesniffer-composer-installer": "^1", | ||
| "drevops/behat-phpserver": "^2.1.1", | ||
| "drevops/behat-screenshot": "^2.1", | ||
| "drupal/coder": "^8.3.28", | ||
| "dvdoug/behat-code-coverage": "^5.3.2.1", | ||
| "ergebnis/composer-normalize": "^2.47", | ||
| "mglaman/phpstan-drupal": "^1.3.7", 10000 | ||
| "php-parallel-lint/php-parallel-lint": "^1.4", | ||
| "phpcompatibility/php-compatibility": "^9.3.5", | ||
| "phpspec/prophecy-phpunit": "^2.3", | ||
| "phpstan/extension-installer": "^1.4.3", | ||
| "phpstan/phpstan": "^1.12.25", | ||
| "phpunit/phpunit": "^11", | ||
| "rector/rector": "^1.2" | ||
| }, |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider pinning dev-tools to patch versions to keep CI deterministic
Almost every entry in "require-dev" is specified with a loose ^ constraint (major version float).
Because these packages are used only for QA / CI, unexpected minor/patch upgrades can suddenly break the pipeline (PHPUnit, Rector, PHP-Stan, etc.). A common practice is:
- lock large tools to an exact patch version (
"phpunit/phpunit": "11.1.2"), - bump them deliberately in a dedicated “Update tools” PR.
This keeps builds reproducible and makes regressions easier to bisect.
If you prefer the current policy, feel free to ignore.
| public function testCamelToSnake(string $input, string $expected, string $separator = '_'): void { | ||
| $actual = camel_to_snake($input, $separator); | ||
| $this->assertEquals($expected, $actual); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Prefer assertSame() for string comparisons to avoid type juggling
assertEquals() performs loose comparison; for pure strings the strict variant is safer and yields clearer diff output.
-$this->assertEquals($expected, $actual);
+$this->assertSame($expected, $actual);Apply to all string-only assertions in testCamelToSnake, testArrayToMarkdownTable, and testReplaceContent.
Also applies to: 313-315, 652-654
| $src_file_path = $base_path . DIRECTORY_SEPARATOR . 'src/MissingTrait.php'; | ||
| @unlink($src_file_path); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Avoid silencing errors with @unlink
The error-control operator suppresses useful diagnostics if the file cannot be removed.
Either skip the call when the file does not exist or assert its success:
-@unlink($src_file_path);
+if (file_exists($src_file_path)) {
+ self::assertTrue(unlink($src_file_path), "Failed to remove $src_file_path");
+}📝 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.
| $src_file_path = $base_path . DIRECTORY_SEPARATOR . 'src/MissingTrait.php'; | |
| @unlink($src_file_path); | |
| $src_file_path = $base_path . DIRECTORY_SEPARATOR . 'src/MissingTrait.php'; | |
| if (file_exists($src_file_path)) { | |
| self::assertTrue(unlink($src_file_path), "Failed to remove $src_file_path"); | |
| } |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## 3.x #383 +/- ##
=======================================
Coverage 81.57% 81.57%
=======================================
Files 31 31
Lines 1677 1677
=======================================
Hits 1368 1368
Misses 309 309 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
14b6ef2 to
cfb3cd5
Compare
Summary by CodeRabbit
New Features
Chores
.gitignoreto exclude PHPUnit cache files.Refactor
Documentation