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.

Sign up for GitHub

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

Jump to bottom

Upgrade to PHPUnit v10 #741

Merged
merged 5 commits into from
Feb 5, 2025
Merged

Upgrade to PHPUnit v10 #741

merged 5 commits into from
Feb 5, 2025

Conversation

lcobucci
Copy link
Contributor

@lcobucci lcobucci commented Mar 2, 2023

Fix #740

This makes the test suite compatible with the latest PHPUnit version and upgrades everything to it, which removes the SEGFAULTs we were having previously.

(the first commit is from #737, a rebase to sync is more than enough to remove it from the last merged branch)

@lcobucci
Copy link
Contributor Author

lcobucci commented Mar 2, 2023

Replaces #736

@lcobucci lcobucci force-pushed the upgrade-to-phpunit10 branch from 20cd2c1 to d72bef7 Compare March 2, 2023 00:27
@Ocramius
Copy link
Member

Ocramius commented Mar 2, 2023

Argh, this is possibly blocked by laminas/laminas-ci-matrix-action#189 and laminas/laminas-continuous-integration-action#130

@Ocramius Ocramius added the bug label Mar 2, 2023
@Ocramius Ocramius added this to the 8.4.0 milestone Mar 2, 2023
@lcobucci
Copy link
Contributor Author

lcobucci commented Mar 2, 2023

Argh, this is possibly blocked by laminas/laminas-ci-matrix-action#189 and laminas/laminas-continuous-integration-action#130

Fun!

We can "hide" the SEGFAULTs by temporarily relying on exec().

Trade-offs, eh!?

@lcobucci
Copy link
Contributor Author

lcobucci commented Mar 2, 2023

Can't we add it as extension to be installed for this package (at least until that addressed on the upstream)?

Alternatively we can also use a pre-install script to run arbitrary commands as root: https://github.com/laminas/laminas-continuous-integration-action/blob/1.33.x/entrypoint.sh#L162

That might help to only install a coverage driver for specific jobs and allow moving this forward.

@Ocramius
Copy link
Member

Ocramius commented Mar 2, 2023

I really just need to do the upstream releasing/fixing: however you put the problem, the blocker is me and my constantly clogged schedule :S

@Ocramius
Copy link
Member

Ocramius commented Mar 2, 2023

I'd be fine with using exec() BTW: the code is well tested anyway, so it's an implementation change only.

@lcobucci
Copy link
Contributor Author

lcobucci commented Mar 2, 2023

I'd be fine with using exec() BTW: the code is well tested anyway, so it's an implementation change only.

I'll send a different PR to tackle this, I've the changes in my local history 👍

@lcobucci
Copy link
Contributor Author

lcobucci commented Mar 2, 2023

I'd be fine with using exec() BTW: the code is well tested anyway, so it's an implementation change only.

I'll send a different PR to tackle this, I've the changes in my local history +1

Scratch that, we can already enable PCOV here and solve the issue.

I've pushed an extra commit and rebased the baseline PR to verify that CI works for both.

@Ocramius @asgrim please approve the workflow run (whenever) on both PRs to make sure things are fine 👍

@lcobucci
Copy link
Contributor Author

lcobucci commented Mar 3, 2023

Silly me... I've overlooked the generated command for infection on the matrix 🤦
I used vendor/bin/infection to test, CI is using phpdbg -qrr ./vendor/bin/roave-infection-static-analysis-plugin

Well, that would never work 🤣

@lcobucci
Copy link
Contributor Author

lcobucci commented Mar 3, 2023

All the things I can think of are horrendous... let's release CI-action v2 😆

@Ocramius
Copy link
Member

Ocramius commented Mar 3, 2023

Yeah, and phpdbg + pcov together do lead to segfaults :S

@Ocramius
Copy link
Member

Ocramius commented Mar 3, 2023

@lcobucci doing some paid work, then picking up OCI-CatalogService again

@asgrim asgrim mentioned this pull request Mar 14, 2023
Merged
@Ocramius Ocramius removed this from the 8.4.0 milestone Nov 25, 2023
@lcobucci
Copy link
Contributor Author

@Ocramius should we close this or bump to v11? 😅

@Ocramius
Copy link
Member

If it works, we can merge here, although no idea if it does :P

My OSS efforts have been almost zero this year, sorry.

@lcobucci
Ensure GPG isn't used on tests
0d7cd7c 
Some people - like me - have a tweaked/opinionated global Git
configuration that forces GPG signatures everywhere, which basically
breaks the tests.

This standardises the expected configuration for tests, preventing
unexpected breakages.

Signed-off-by: Luís Cobucci <[email protected]>
@lcobucci lcobucci changed the base branch from 8.4.x to 8.11.x February 3, 2025 19:33
@lcobucci
Ensure data providers are static
746ae62 
PHPUnit 10.x only accepts static data providers, so...

Signed-off-by: Luís Cobucci <[email protected]>
@lcobucci lcobucci force-pushed the upgrade-to-phpunit10 branch from d5789e6 to 8e95060 Compare February 3, 2025 19:57
@lcobucci
Copy link
Contributor Author

lcobucci commented Feb 3, 2025

@Ocramius I had some time here... shall we try again?

@lcobucci lcobucci force-pushed the upgrade-to-phpunit10 branch 2 times, most recently from 3d95673 to 8120fb9 Compare February 3, 2025 20:00
@lcobucci lcobucci force-pushed the upgrade-to-phpunit10 branch from 8120fb9 to 64d860d Compare February 4, 2025 08:17
@lcobucci
Copy link
Contributor Author

lcobucci commented Feb 4, 2025

@Ocramius all the checks should be fine, except for Infection as it still requires laminas-ci-matrix-action 2 to be released (drop phpdbg and use pcov).

@Ocramius
Copy link
Member

Ocramius commented Feb 4, 2025

@lcobucci should we perhaps skip running it via laminas-ci-matrix-* there, and run roave/infection-static-analysis-plugin (locked) ourselves?

@lcobucci lcobucci force-pushed the upgrade-to-phpunit10 branch 2 times, most recently from ec96dc1 to 7d3abac Compare February 4, 2025 19:33
@lcobucci
Copy link
Contributor Author

lcobucci commented Feb 4, 2025

@Ocramius I'm not that familiar with the config there, can you check if 7d3abac handles it?

@Ocramius
Copy link
Member

Ocramius commented Feb 4, 2025

@lcobucci see https://github.com/laminas/laminas-ci-matrix-action/blob/7f7976e6faf1ee8fcfca77cf0487cc4d0b486f11/laminas-ci.schema.json

Ocramius added a commit to Ocramius/DoctrineBatchUtils that referenced this pull request Feb 5, 2025
@Ocramius
Trying to disable automatic CI pipeline, which tries to use phpdbg …
d0b6f58 
…for coverage

Trying to chase @lcobucci's work on Roave/BackwardCompatibilityCheck#741

See schema:

* https://github.com/laminas/laminas-ci-matrix-action/blob/7f7976e6faf1ee8fcfca77cf0487cc4d0b486f11/laminas-ci.schema.json

This commit should:

* remove automatic infection pipeline entry
* add infection run via `pcov` coverage driver

Fixes:

```
 Error: ] Project tests must be in a passing state before running Infection.
         Infection runs the test suite in a RANDOM order. Make sure your tests
         do not have hidden dependencies.

         You can add these attributes to `phpunit.xml` to check it: <phpunit
         executionOrder="random" resolveDependencies="true" ...

         If you don't want to let Infection run tests in a random order, set the
         `executionOrder` to some value, for example <phpunit
         executionOrder="default"

         Check the executed command to identify the problem:
         '/usr/bin/phpdbg8.2' '-qrr' '/github/workspace/vendor/bin/phpunit'
         '--configuration'
         '/tmp/infection/phpunitConfiguration.initial.infection.xml'
         '--coverage-xml=/tmp/infection/coverage-xml'
         '--log-junit=/tmp/infection/junit.xml'
         PHPUnit reported an exit code of 1.
```
Ocramius added a commit to Ocramius/DoctrineBatchUtils that referenced this pull request Feb 5, 2025
@Ocramius
Trying to disable automatic CI pipeline, which tries to use phpdbg …
caaea2b 
…for coverage

Trying to chase @lcobucci's work on Roave/BackwardCompatibilityCheck#741

See schema:

* https://github.com/laminas/laminas-ci-matrix-action/blob/7f7976e6faf1ee8fcfca77cf0487cc4d0b486f11/laminas-ci.schema.json

This commit should:

* remove automatic infection pipeline entry
* add infection run via `pcov` coverage driver

Fixes:

```
 Error: ] Project tests must be in a passing state before running Infection.
         Infection runs the test suite in a RANDOM order. Make sure your tests
         do not have hidden dependencies.

         You can add these attributes to `phpunit.xml` to check it: <phpunit
         executionOrder="random" resolveDependencies="true" ...

         If you don't want to let Infection run tests in a random order, set the
         `executionOrder` to some value, for example <phpunit
         executionOrder="default"

         Check the executed command to identify the problem:
         '/usr/bin/phpdbg8.2' '-qrr' '/github/workspace/vendor/bin/phpunit'
         '--configuration'
         '/tmp/infection/phpunitConfiguration.initial.infection.xml'
         '--coverage-xml=/tmp/infection/coverage-xml'
         '--log-junit=/tmp/infection/junit.xml'
         PHPUnit reported an exit code of 1.
```
@Ocramius
Copy link
Member

Ocramius commented Feb 5, 2025

I managed to get things running in a similar way here: Ocramius/DoctrineBatchUtils#392

That schema file is a blessing :D

Ocramius added a commit to Ocramius/PackageVersions that referenced this pull request Feb 5, 2025
@Ocramius
Use own Infection pipeline for PHP 8.4, since PHPUnit dropped PHPDBG …
139677d 
…coverage support

Ref: https://github.com/laminas/laminas-ci-matrix-action/blob/7f7976e6faf1ee8fcfca77cf0487cc4d0b486f11/laminas-ci.schema.json
Ref: Ocramius/DoctrineBatchUtils#392
Ref: Roave/BackwardCompatibilityCheck#741
Ref: laminas/laminas-continuous-integration-action#130
Ref: laminas/laminas-ci-matrix-action#189
Ref: sebastianbergmann/php-code-coverage#945
Ref: sebastianbergmann/php-code-coverage@c304be7
This was referenced Feb 5, 2025
Merged
Merged
@lcobucci
Override Infection CI job definition
d298f72 
Given the plans for laminas-continuous-integration-action v2 is to always
ship PCOV, we can simply enable it here to allow us to detach the two
processes.

This temporarily disables the default infection job and provides a new
configuration that uses PCOV instead.

Signed-off-by: Luís Cobucci <[email protected]>
@lcobucci lcobucci force-pushed the upgrade-to-phpunit10 branch from 7d3abac to d298f72 Compare February 5, 2025 19:11
@lcobucci
Copy link
Contributor Author

lcobucci commented Feb 5, 2025

@Ocramius the schema file helps a lot 👍

I took a slightly different approach so that we always run against the minimum supported PHP version (according to https://github.com/laminas/laminas-ci-matrix-action?tab=readme-ov-file#job-element it should work).

I think everything will just work 😄

@Ocramius
Copy link
Member

Ocramius commented Feb 5, 2025

I think everything will just work 😄

We do this not because it is easy, but because we believed it would be easy.

@Ocramius Ocramius added this to the 8.11.0 milestone Feb 5, 2025
@Ocramius Ocramius removed the bug label Feb 5, 2025
@Ocramius Ocramius self-assigned this Feb 5, 2025
@Ocramius
Copy link
Member

Ocramius commented Feb 5, 2025

🚢 thanks @lcobucci!

@Ocramius Ocramius merged commit eed5a57 into Roave:8.11.x Feb 5, 2025
13 checks passed
@lcobucci lcobucci deleted the upgrade-to-phpunit10 branch February 5, 2025 19:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Ocramius Ocramius

Labels
enhancement
Projects
None yet
Milestone
8.11.0
Development

Successfully merging this pull request may close these issues.

SEGFAULTs consistently happening on CI
2 participants
@lcobucci @Ocramius