Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhancement: Build phpunit-slow-test-detector.phar #273

Merged
merged 1 commit into from
May 30, 2023
Merged

Conversation

localheinz
Copy link
Member

This pull request

  • builds ergebnis/phpunit-slow-test-detector as a PHAR

@codecov
Copy link

codecov bot commented May 30, 2023

Codecov Report

Merging #273 (2244571) into main (8a5bca1) will not change coverage.
The diff coverage is n/a.

❗ Current head 2244571 differs from pull request most recent head d59456d. Consider uploading reports for the commit d59456d to get more accurate results

@@            Coverage Diff            @@
##               main     #273   +/-   ##
=========================================
  Coverage     94.64%   94.64%           
  Complexity       61       61           
=========================================
  Files            15       15           
  Lines           299      299           
=========================================
  Hits            283      283           
  Misses           16       16           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@localheinz localheinz force-pushed the feature/phar branch 7 times, most recently from 2244571 to 0b8bf19 Compare May 30, 2023 17:06
manifest.xml Outdated
<?xml version="1.0" encoding="utf-8" ?>
<phar xmlns="https://phar.io/xml/manifest/1.0">
<contains name="ergebnis/phpunit-slow-test-detector" version="2.2.0" type="extension">
<extension for="phpunit/phpunit" compatible="^10.0.0"/>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sebastianbergmann

Interestingly, when I apply the following patch

Suggested change
<extension for="phpunit/phpunit" compatible="^10.0.0"/>
<extension for="phpunit/phpunit" compatible="^10.0.1"/>

and run

make phar

fails with

.phive/phpunit --configuration=test/Phar/phpunit.xml
PHPUnit 10.0.19 by Sebastian Bergmann and contributors.

Error while bootstrapping extension: Class "Ergebnis\PHPUnit\SlowTestDetector\Extension" does not exist
make: *** [phar] Error 2

@localheinz localheinz force-pushed the feature/phar branch 2 times, most recently from f060740 to d59456d Compare May 30, 2023 17:13
@localheinz localheinz merged commit 0a85826 into main May 30, 2023
@localheinz localheinz deleted the feature/phar branch May 30, 2023 17:15
@alfredbez
Copy link

alfredbez commented May 30, 2023

I build this locally and copied that to our project. When I try to run the tests, the following error is shown:

Error while bootstrapping extension: Class "Ergebnis\PHPUnit\SlowTestDetector\Extension" does not exist

The same happens when I download the phar via phive:

phive install https://github.com/ergebnis/phpunit-slow-test-detector/releases/download/2.3.0/phpunit-slow-test-detector.phar

@localheinz
Copy link
Member Author

@alfredbez

Which version of PHPUnit does the PHAR have?

<extension for="phpunit/phpunit" compatible="^10.1"/>
</contains>

<copyright>
Copy link
Member Author

@localheinz localheinz May 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sebastianbergmann @theseer

Not sure how this works, but when I change

-<extension for="phpunit/phpunit" compatible="^10.1"/>
+<extension for="phpunit/phpunit" compatible="^10.1.3"/>

I end up with the error that @alfredbez describes.

In other words, somewhere in https://github.com/sebastianbergmann/phpunit/blob/46739478930289fa955fe4697688bae921a2674a/src/Runner/Extension/PharLoader.php#L56-L76, the PharLoader just continues instead of requiring the file containing the PHAR.

Looks like the compatible attribute has issues when using major.minor.patch versions.

Do you have an idea how to fix this?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it has indeed issues, that would be a bug in the manifest component. But what confuses me most is that it doesn't report that as a problem. It should report the fact it's not loading that extension.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's an internal issue within PHPUnit: Currently, only the Version::series() is passed on the version match, meaning you cannot currently require a patch level version.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be a separate issue in PHPUnit that also affects PHPUnit 8.5 and PHPUnit 9.6.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can have a look on side effects later today or tomorrow. If anyone opens that ticket, feel free to assign me :)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sebastianbergmann/phpunit@284eedd should fix this.

Sidenote: just this morning I added tests for unhappy code paths in extension bootstrapping for PHPUnit 10. The next thing I wanted to write tests for was loading of extensions from PHP archives ...

@alfredbez
Copy link

I've tested this with these versions and all of them produce the same issue:

  • phpunit-10.0.0.phar
  • phpunit-10.0.1.phar
  • phpunit-10.0.19.phar
  • phpunit-10.1.0.phar
  • phpunit-10.1.1.phar
  • phpunit-10.1.2.phar
  • phpunit-10.1.3.phar

I have installed PHPUnit first via phive:

phive install phpunit --copy

Then I've installed this extension as mentioned in the previous comment. I try to run the tests via tools/phpunit.

My phpunit.xml.dist looks like this:

<?xml version="1.0" encoding="UTF-8"?>
<phpunit xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
         xsi:noNamespaceSchemaLocation="https://schema.phpunit.de/10.1/phpunit.xsd"
         bootstrap="phpunit-bootstrap.php"
         executionOrder="depends,defects"
         beStrictAboutOutputDuringTests="true"
         colors="true"
         testdox="false"
         displayDetailsOnIncompleteTests="true"
         displayDetailsOnSkippedTests="true"
         displayDetailsOnTestsThatTriggerDeprecations="true"
         displayDetailsOnTestsThatTriggerErrors="true"
         displayDetailsOnTestsThatTriggerNotices="true"
         displayDetailsOnTestsThatTriggerWarnings="true"
         cacheDirectory=".phpunit.cache"
         requireCoverageMetadata="false"
         beStrictAboutCoverageMetadata="false">
    <extensions>
        <bootstrap class="Ergebnis\PHPUnit\SlowTestDetector\Extension">
            <parameter name="maximum-duration" value="250"/>
        </bootstrap>
    </extensions>
    <coverage/>
    <testsuites>
        <testsuite name="default">
            <directory suffix="Test.php">tests/PyzUnitTest</directory>
        </testsuite>
    </testsuites>
    <source>
        <include>
            <directory suffix=".php">src</directory>
        </include>
        <exclude>
            <directory suffix=".php">src/Orm</directory>
            <directory suffix=".php">src/Generated</directory>
        </exclude>
    </source>
</phpunit>

@localheinz
Copy link
Member Author

@alfredbez

Are you using the extensionsDirectory attribute to configure the directory in which the PHPUnit extensions reside?

@alfredbez
Copy link

I just stumbled upon that and tried to set this, but had no luck so far. My phpunit.xml.dist is in the root directory of my repository, both phpunit and the extension are installed as PHARs in the tools folder.

├── src
├── vendor
├── composer.json
├── composer.lock
├── phpunit.xml.dist
└── tools
    ├── phpunit
    └── phpunit-slow-test-detector

What should be the value for extensionsDirectory? tools, ../tools, ., or something different? Is it even ok to have both PHARs in the same directory?

@theseer
Copy link

theseer commented May 31, 2023

That won't work: PHPUnit does not recognize extensions without the .phar extension and - as it seems - when they're symlinked. I consider at least the later a bug. (/cc @sebastianbergmann)

@sebastianbergmann
Copy link

Please open an issue for PHPUnit then (if that is the case).

@theseer
Copy link

theseer commented May 31, 2023

I can't get the extension to work: When I clone this repository and run make phar I get errors because I don't have a global composer available. When I "just" run the box compile command (what a broken term for creating an archive, but okay), I do get a phar but trying to load that i get a hard crash:

PHP Warning:  require_once(phar:///home/theseer/storage/php/templado/engine/tools/phpunit.d/phpunit-slow-test-detector.phar/phar/../vendor/autoload.php): Failed to open stream: phar error: "vendor/autoload.php" is not a file in phar "/home/theseer/storage/php/templado/engine/tools/phpunit.d/phpunit-slow-test-detector.phar" in phar:///home/theseer/storage/php/templado/engine/tools/phpunit.d/phpunit-slow-test-detector.phar/phar/phpunit-slow-test-detector.php on line 14


An error occurred inside PHPUnit.

Message:  Failed opening required 'phar:///home/theseer/storage/php/templado/engine/tools/phpunit.d/phpunit-slow-test-detector.phar/phar/../vendor/autoload.php' (include_path='.:/usr/share/pear:/usr/share/php:/usr/share/pear:/usr/share/php')
Location: phar:///home/theseer/storage/php/templado/engine/tools/phpunit.d/phpunit-slow-test-detector.phar/phar/phpunit-slow-test-detector.php:14

#0 /home/theseer/storage/php/templado/engine/tools/phpunit.d/phpunit-slow-test-detector.phar(14): require()
#1 /home/theseer/storage/php/phpunit/phpunit/src/Runner/Extension/PharLoader.php(84): require('...')
#2 /home/theseer/storage/php/phpunit/phpunit/src/TextUI/Application.php(115): PHPUnit\Runner\Extension\PharLoader->loadPharExtensionsInDirectory()
#3 /home/theseer/storage/php/phpunit/phpunit/phpunit(99): PHPUnit\TextUI\Application->run()
#4 {main}

That crash probably qualifies as its own bug in PHPUnit ;-) but I'm at a loss. It tried to load the extension though, despite the 10.1.3 compatible attribute.

@sebastianbergmann
Copy link

FYI: https://github.com/sebastianbergmann/phpunit/tree/10.1/build/test-extension is used to generate https://github.com/sebastianbergmann/phpunit/tree/10.1/tests/end-to-end/_files/phar-extension/tools/phpunit.d which is used in PHPUnit's own test suite for testing the loading of PHAR extensions.

@theseer
Copy link

theseer commented May 31, 2023

For the record:

  • The building issue (Enhancement: Build phpunit-slow-test-detector.phar #273 (comment)) seems to be related to not having a global composer install.

  • Upon further testing, I cannot seem to reproduce my issue with symlinks. I consider that a mis-interpretation while debugging

  • Using phive to install extensions currently is unsupported, as phive would symlink the file without .phar extension, making phpunit ignore the file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants