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

Keep full PHPUnit 9.x support #417

Merged
merged 1 commit into from
Dec 16, 2023
Merged

Conversation

mvorisek
Copy link
Contributor

@mvorisek mvorisek commented Dec 14, 2023

revert #416

"run in separate process" has an issue on PHP 8.3+

the issue has nothing to do with this package compatibility, phpunit 9.x is fully compatible

related with #368 and #369

Copy link

codecov bot commented Dec 14, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (3ddec39) 66.66% compared to head (2f86dfa) 66.73%.

Additional details and impacted files
@@             Coverage Diff              @@
##               main     #417      +/-   ##
============================================
+ Coverage     66.66%   66.73%   +0.07%     
  Complexity       92       92              
============================================
  Files            21       21              
  Lines           459      460       +1     
============================================
+ Hits            306      307       +1     
  Misses          153      153              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mvorisek mvorisek changed the title Keep full 9.x support Keep full PHPUnit 9.x support Dec 14, 2023
@localheinz
Copy link
Member

But why, @mvorisek?

Who in their right mind will not update phpunit/phpunit:^9.0.0 to the latest version of phpunit/phpunit:^9.6.15?

@localheinz localheinz self-assigned this Dec 15, 2023
@mvorisek
Copy link
Contributor Author

Imagine project like this one /w slow test detector dependency.

If this package/dependency will limit the phpunit version, lowest testing will be constrained more than needed. Therefore this PR.

@localheinz
Copy link
Member

If this package/dependency will limit the phpunit version, lowest testing will be constrained more than needed. Therefore this PR.

Again, why would anyone want to use phpunit/phpunit:9.0.0 when they could use

  • phpunit/phpunit:9.0.1
  • phpunit/phpunit:9.1.0
  • phpunit/phpunit:9.6.15

I can't see a problem with limiting use to versions of phpunit/phpunit that do not have known bugs.

And, when we start patching phpunit/phpunit in test runs, where do we stop?

@mvorisek
Copy link
Contributor Author

Normally, phpunit/phpunit package is added/used as dev package where is does not matter much. But when this/slow-detector package is used together with some project requiring phpunit/phpunit package as non-dev, such project should not be limited by requiring slow detector. So please, let's keep full 9.x and 10.x support. The slow detector is 100% compatible.

@localheinz
Copy link
Member

@mvorisek

Do you have examples of such packages?

@mvorisek
Copy link
Contributor Author

mvorisek commented Dec 16, 2023

In short any PHPUnit lib. composer.json example:

{
    "require": {
        "php": ">=7.4 <8.4",
        "phpunit/phpunit": "^9 || ^10"
    },
    "require-dev": {
        "ergebnis/phpunit-slow-test-detector": "^2.4"
    },
}

When "lowest" packages are installed for PHPUnit testing, the ergebnis/phpunit-slow-test-detector must have zero or minimal constraints, otherwise the trully tested packages will be different than the trully supported.

@localheinz
Copy link
Member

@mvorisek

I have understood, but I would like to see an actual package where that would be a problem to avoid solving problems that aren't problems.

@mvorisek
Copy link
Contributor Author

I am describing our internal testing package. With the composer.json above and composer update --prefer-lowest command PHPUnit 9.0.0 must be installed. This is what this PR does.

This/slow detector package is thankfully fully compatible. We can avoid the patching by testing PHP 8.3 /w higher PHPUnit, but I have choosen to patch PHPUnit to make the testing (of this package) as best as possible.

@localheinz
Copy link
Member

@mvorisek

I am describing our internal testing package. With the composer.json above and composer update --prefer-lowest command PHPUnit 9.0.0 must be installed. This is what this PR does.

And which of your internal packages or applications that use your internal testing package really need to use phpunit/phpunit:9.0.0 and why?

@localheinz
Copy link
Member

@mvorisek

Perhaps your company can request an invoice for accepting this patch - and if you want me to, I can choose an amount that

  • either covers the pain of accepting such a patch
  • or is high enough that your company stops using phpunit/phpunit:9.0.0 and instead uses at least phpunit/phpunit:9.4.3

By the way, bug fix support for phpunit/phpunit:^9 ends soon, too:

CleanShot 2023-12-16 at 16 33 09@2x

@mvorisek
Copy link
Contributor Author

And which of your internal packages or applications that use your internal testing package really need to use phpunit/phpunit:9.0.0 and why?

#417 (comment) is strong example. When a package claims it does support some deps, the deps must be really supported and tested. Dev packages must imply minimal side effect to the testing, in this case, not prohibit to install PHPUnit 9.0.0 - 9.4.2.

@localheinz
Copy link
Member

@mvorisek

So you will not be requesting an invoice?

@mvorisek
Copy link
Contributor Author

Andreas, thank you for bringing this topic up again and addressing the elephant in the room. I saw your ergebnis/composer-normalize#1236 (comment) comment where you came up with the financing initially.

In OOS I play about the same role you do. I am developer, researcher and influecer. I understand that you would welcome big corporations to take a part of the costs you have to keep the projects maintained. I am however not a big corporation. If it would help you, I can pay you a few cups of coffee from my personal budget though.

What I can do and I am already doing is to bring more people to you, potentially working in the big corporations. In numbers, I have spreaded the word of your repos, mainly for https://github.com/ergebnis/composer-normalize and newly for this one, to thousands of people, and even added these to popular repos like PrestaShop, Roundcube, atk4, ... Besides my direct contributions, this is a support I do for you and will happily continue to do to participate on your bills and projects this way.

Co-authored-by: Andreas Möller <[email protected]>
Co-authored-by: Michael Voříšek <[email protected]>
@localheinz
Copy link
Member

Well then, let's get it merged, @mvorisek!

@localheinz localheinz merged commit 4bf3753 into ergebnis:main Dec 16, 2023
39 of 40 checks passed
@localheinz
Copy link
Member

Thank you, @mvorisek!

@mvorisek
Copy link
Contributor Author

Thank you ❤

@mvorisek mvorisek deleted the phpunit_9_0_0 branch December 17, 2023 01:15
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.

2 participants