Merge pull request #672 from phparkitect/claude/lowest-dependency-test-matrix-e7cxf3

Test the lowest allowed dependency set in CI

Author: fain182

.github/workflows/build.yml

@@ -17,6 +17,14 @@ jobs:
       fail-fast: false
       matrix:
         php-versions: [ '8.0', '8.1', '8.2', '8.3', '8.4', '8.5' ]
+        dependencies: [ 'highest' ]
+        include:
+          # The lowest allowed dependency set needs to be verified only once, and the
+          # oldest supported PHP is where consumers are most likely to end up with it.
+          - php-versions: '8.0'
+            dependencies: 'lowest'
+
+    name: "PHP ${{ matrix.php-versions }} (${{ matrix.dependencies }} deps)"
 
     steps:
     - uses: actions/checkout@v7
@@ -40,20 +48,35 @@ jobs:
         path: |
           vendor
           tools/psalm/vendor
-        key: ${{ runner.os }}-php-${{ hashFiles('**/composer.lock') }}
+        # composer.lock is not committed, so it cannot take part in the cache key:
+        # the installed set depends on the PHP version and on the resolution
+        # strategy, and both must be part of the key to keep the caches separate
+        key: ${{ runner.os }}-php-${{ matrix.php-versions }}-${{ matrix.dependencies }}-${{ hashFiles('composer.json') }}
         restore-keys: |
-          ${{ runner.os }}-php-
+          ${{ runner.os }}-php-${{ matrix.php-versions }}-${{ matrix.dependencies }}-
 
     - name: Install dependencies
+      if: ${{ matrix.dependencies == 'highest' }}
       run: composer install --prefer-dist
 
+    # Without a committed composer.lock every other job resolves to the newest
+    # allowed release, so the lower bounds declared in composer.json are never
+    # exercised. This job installs the other end of those ranges: the set a
+    # consumer with older pinned dependencies actually gets.
+    - name: Install lowest allowed dependencies
+      if: ${{ matrix.dependencies == 'lowest' }}
+      run: composer update --prefer-dist --prefer-lowest --prefer-stable
+
     # Psalm 6 needs PHP >= 8.1 to run; the analysis target stays 8.0 (phpVersion in psalm.xml)
     - name: Static Analysis
       if: ${{ matrix.php-versions  == '8.1' }}
       run: |
         composer install --prefer-dist -d tools/psalm
         tools/psalm/vendor/bin/psalm --no-cache
 
+    # Both dependency sets report coverage: code that supports one version of a
+    # dependency is dead code under the other one, so a single set can never cover
+    # all of src. Codecov merges the two reports, and codecov.yml waits for both.
     - name: Test with coverage
       if: ${{ matrix.php-versions  == '8.0' }}
       run: ./bin/phpunit -d memory_limit=-1 --coverage-clover clover.xml
@@ -67,6 +90,7 @@ jobs:
       uses: codecov/codecov-action@303a32d7a59b442fa8d48b6a1cc6825c09c847a5 # v7.1.1
       with:
         token: ${{ secrets.CODECOV_TOKEN }}
+        flags: ${{ matrix.dependencies }}
 
   phar:
     runs-on: "ubuntu-22.04"

CONTRIBUTING.md

@@ -50,3 +50,22 @@ make csfix     # run code style fixer
 make psalm     # run static analysis
 ```
 
+### Testing against the lowest allowed dependencies
+
+`composer.lock` is not committed, so `composer install` always gives you the
+newest release allowed by `composer.json`. Arkitect is installed as a dependency
+of other projects, though, where the resolver often picks something older, so CI
+also runs the test suite against the lower bound of every declared constraint.
+
+To reproduce that job locally:
+
+```shell
+composer update --prefer-lowest --prefer-stable
+make test
+composer update            # restore the newest allowed dependencies
+```
+
+If a change needs a feature that is not in the lowest allowed version of a
+dependency, raise the constraint in `composer.json` instead of working around
+it.
+

codecov.yml

@@ -29,13 +29,27 @@ comment:
   layout: "reach, diff, flags, files"
   behavior: default
   require_changes: false
+  # Same reason as codecov.notify.after_n_builds below.
+  after_n_builds: 2
 
-# Wait for the (single) coverage upload before computing the status, so the
-# check doesn't run against a partial report.
+# Wait for both coverage uploads before computing the status, so the check
+# doesn't run against a partial report. The build uploads one report per
+# dependency set (see the `flags` below): the code supporting one version of a
+# dependency is dead code under the other, so only the merge of the two reports
+# describes the coverage of src.
 codecov:
   require_ci_to_pass: true
   notify:
     wait_for_ci: true
+    after_n_builds: 2
+
+# The flags exist to tell the two uploads apart, not to gate them separately:
+# neither dependency set covers src on its own, so a per-flag status would be
+# meaningless. Nothing is carried forward from older commits either, both
+# reports are uploaded on every build.
+flag_management:
+  default_rules:
+    carryforward: false
 
 ignore:
   - "tests"

composer.json

@@ -23,16 +23,16 @@
     ],
     "require": {
         "php": "^8.0",
-        "nikic/php-parser": "~5",
+        "nikic/php-parser": "^5.3",
         "ondram/ci-detector": "^4.2",
-        "phpstan/phpdoc-parser": "^1.2|^2.0",
+        "phpstan/phpdoc-parser": "^1.20|^2.0",
         "symfony/console": "^5.4|^6.0|^7.0|^8.0",
         "symfony/finder": "^5.4|^6.0|^7.0|^8.0",
         "webmozart/assert": "^1.12|^2.0"
     },
     "require-dev": {
         "friendsofphp/php-cs-fixer": "^3.95",
-        "mikey179/vfsstream": "^1.6",
+        "mikey179/vfsstream": "^1.6.8",
         "phpspec/prophecy": "^1.10",
         "phpspec/prophecy-phpunit": "^2.5",
         "phpunit/phpunit": "^9.6|^10.0|^11.0",

src/Analyzer/DocblockParserFactory.php

@@ -12,15 +12,19 @@
 class DocblockParserFactory
 {
     /**
+     * Psalm only ever sees one of the two installed versions of phpstan/phpdoc-parser,
+     * so the constructor calls of the other branch always look wrong to it.
+     *
      * @psalm-suppress TooFewArguments
+     * @psalm-suppress TooManyArguments
      * @psalm-suppress InvalidArgument
      */
     public static function create(): DocblockParser
     {
         $phpDocParser = null;
         $phpDocLexer = null;
 
-        // this if is to allow using v 1.2 or v2
+        // this if is to allow using v1 or v2
         if (class_exists(ParserConfig::class)) {
             $parserConfig = new ParserConfig(['lines' => true]);
             $constExprParser = new ConstExprParser($parserConfig);
@@ -30,7 +34,10 @@ public static function create(): DocblockParser
         } else {
             $typeParser = new TypeParser();
             $constExprParser = new ConstExprParser();
-            $phpDocParser = new PhpDocParser($typeParser, $constExprParser);
+            // on v1 line numbers are opt-in through $usedAttributes, the 5th constructor
+            // argument: without it every node reports no line and violations would be
+            // reported on the wrong line
+            $phpDocParser = new PhpDocParser($typeParser, $constExprParser, false, false, ['lines' => true]);
             $phpDocLexer = new Lexer();
         }