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

Feature/allow phpunit 10 #841

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Chris53897
Copy link

The 2 abstract Test-Classes are excludet to nor raise Warnings and break CI-runs.

@codecov
Copy link

codecov bot commented May 12, 2023

Codecov Report

Merging #841 (f04540b) into master (19e5890) will decrease coverage by 0.14%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master     #841      +/-   ##
============================================
- Coverage     98.47%   98.33%   -0.14%     
+ Complexity      345      344       -1     
============================================
  Files            23       23              
  Lines           983      903      -80     
============================================
- Hits            968      888      -80     
  Misses           15       15              

see 17 files with indirect coverage changes

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


<listeners>
<listener class="Symfony\Bridge\PhpUnit\SymfonyTestsListener" />
Copy link
Member

Choose a reason for hiding this comment

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

This makes us loose the existing feature where our CI fails if we trigger deprecated APIs, which is something we wanted to have

Copy link
Author

Choose a reason for hiding this comment

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

Yes of course. This --migrate-configuration does remove it, and i did not checked the backup file. I will re-add this

Copy link
Member

Choose a reason for hiding this comment

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

Actually, the symfony/phpunit-bridge is not yet compatible with PHPUnit 10. So this is a blocker for using it in our CI

Copy link
Author

Choose a reason for hiding this comment

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

I just noticed that ;(
I added it, but outcommented the code. Xml-Names changed too, i am not sure if phunit 8 can handle the new Xml-Names.

Copy link
Author

Choose a reason for hiding this comment

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

I guess the best solution is, to wait until the bridge symfony/phpunit-bridge is compatible with PHPUnit 10.
There could be 2 seperate phpunit (for 8/9 and 10) config files and a switch in the ci tests.
But i guess that is not what we want.

Copy link
Member

Choose a reason for hiding this comment

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

Indeed. There is nothing wrong with using PHPUnit 9 in our CI. We don't want to maintain 2 configurations if we can avoid it.

However, the TestCase renaming and the migration to static methods for data-providers are something we could merge even if we don't use PHPUnit 10 yet.

Copy link
Author

Choose a reason for hiding this comment

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

Ok, i will provide a seperate PR. (probably this weekend)

phpunit.xml.dist Outdated
<testsuite name="Behat Mink test suite">
<directory>tests</directory>
<exclude>./tests/Element/ElementTest.php</exclude>
<exclude>./tests/Selector/NamedSelectorTest.php</exclude>
Copy link
Member

Choose a reason for hiding this comment

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

excluding some tests is a no-go. Our testsuite must run.

If those are not actually tests but abstract test cases, the right fix is to rename them rather than having to maintain exclude rules.

Copy link
Author

Choose a reason for hiding this comment

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

These are abstract Tests. I changed the tests. There are of course multiple solutions. Please check if my solution is ok.

@aik099
Copy link
Member

aik099 commented May 18, 2023

This PR also includes the changes from #842 . Not sure if that was planned.

@Chris53897
Copy link
Author

@aik099 Yes this in intended, please see #841 (comment)
This PR could be rebased after the bridge symfony/phpunit-bridge is compatible with PHPUnit 10.

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

Successfully merging this pull request may close these issues.

4 participants