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

Jump to bottom

Run AllTests on PHP8 #16897

Merged
merged 22 commits into from Aug 17, 2021
Merged

Run AllTests on PHP8 #16897

merged 22 commits into from Aug 17, 2021

Conversation

sgiehl
Copy link
Member

@sgiehl sgiehl commented Dec 7, 2020

Description:

Runs AllTests on PHP 8 instead of PHP 7.4

Review

  • Functional review done
  • Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
  • Security review done see checklist
  • Code review done
  • Tests were added if useful/possible
  • Reviewed for breaking changes
  • Developer changelog updated if needed
  • Documentation added if needed
  • Existing documentation updated if needed

@sgiehl sgiehl added the c: Tests & QA For issues related to automated tests or making it easier to QA & test issues. label Dec 7, 2020
@sgiehl sgiehl added this to the 4.1.0 milestone Dec 7, 2020
@sgiehl sgiehl force-pushed the php8tests branch 8 times, most recently from a4f0a32 to 0869f11 Compare December 7, 2020 11:46
@sgiehl
Copy link
Member Author

sgiehl commented Dec 7, 2020

@tsteur the tests are running now on PHP 8. But I needed to update to PHPUnit 9 for that build, as PHPUnit 8.5 doesn't support PHP 8. Unfortunately PHPUnit 9 doesn't support PHP < 7.3, so we can't update in general.

Nevertheless there seem a lot tests failing. Would need some time to investigate that a bit further. Let me know if or when I should plan to do that.

@sgiehl sgiehl force-pushed the php8tests branch 2 times, most recently from ade2b73 to f9482e3 Compare December 7, 2020 15:40
@tsteur
Copy link
Member

tsteur commented Dec 7, 2020

@sgiehl it seems to be mostly working? we could look into it as part of 4.1 if it's somewhat quick to do to fix these issues.

For now priority is 4.0.X and I think I pinged you on few other sec issues etc

@sgiehl
Copy link
Member Author

sgiehl commented Dec 7, 2020

@tsteur Actually might be a bit more. The second build that is shown as success actually silently fails. See https://travis-ci.com/github/matomo-org/matomo/jobs/455444024#L996
Not yet sure why the tests suddenly abort while running.

@sgiehl sgiehl mentioned this pull request Dec 8, 2020
Merged
9 tasks
@sgiehl sgiehl force-pushed the php8tests branch 6 times, most recently from 4bde232 to df86948 Compare December 9, 2020 08:15
@sgiehl sgiehl mentioned this pull request Dec 9, 2020
Merged
9 tasks
@sgiehl sgiehl force-pushed the php8tests branch 2 times, most recently from cc3a2f6 to 3769f1f Compare December 9, 2020 15:31
@sgiehl
Copy link
Member Author

sgiehl commented Dec 10, 2020

Some tests are failing due to adjustments required in TCPDF. Those should be fixed with tecnickcom/TCPDF#293
Waiting for a new release to get that fixed...

@sgiehl sgiehl force-pushed the php8tests branch 4 times, most recently from 109b850 to e8d49b3 Compare December 10, 2020 15:57
@sgiehl sgiehl marked this pull request as ready for review August 16, 2021 13:14
@sgiehl
Copy link
Member Author

sgiehl commented Aug 16, 2021

This should now be ready for a review. Had to change a couple of tests in order to make them run on PHP 7 & PHP 8, but now the only remaining failing tests also fail on 4.x-dev...

Note: Once we raised the requirement for Matomo to PHP 7.3 or higher, we can remove the special handling for PHP 8 tests again and update PHPUnit globally, as newer version of PHPUnit are not compatible with PHP 7.2.

@sgiehl
Run AllTests on PHP8
2739790
@sgiehl
use newer phpunit for tests on PHP8
d80159d
@sgiehl
fix notices/errors
ececf33
@sgiehl
handle trace of phpunit 9
8a9d4e4
@sgiehl
ensure to compare same types for range check
e64bcb1
@sgiehl
adjust expected result for php8
48f5697
@sgiehl
try handling hasDependencies does not exist in PHPUnit 9
c174eff
@sgiehl
fix: ArgumentCountError: mysqli_stmt::bind_param() does not accept un…
1caedca 
…known named parameters
@sgiehl
Fix usercountry tests
27399ef
@sgiehl
remove incorrect date check
c0cd610
@sgiehl
fix expected exception message
1632bf6
@sgiehl
fix referrers api test for php8
a36d2f2
@sgiehl
add test hint
e0461ca
@sgiehl
try to fix session related tests
9c81762
@sgiehl
debug test failure
2df12d9
@sgiehl
try to fix mail tests
87a10d2
@sgiehl
fix filterUser method
8422a4a
@sgiehl
exlucde phpunit from build size calculation
848c1e5
@sgiehl
fix scheduled reports test
3537cb3
@sgiehl
update release checklist test
164d49b
@sgiehl
unset climode
254efb5
@sgiehl
improves some tests
d3b62a5
@tsteur
Copy link
Member

tsteur commented Aug 16, 2021

👍 looks good. Great to have it working 🎉

@sgiehl sgiehl merged commit 0829d6e into 4.x-dev Aug 17, 2021
@sgiehl sgiehl deleted the php8tests branch August 17, 2021 07:59
@justinvelluppillai justinvelluppillai added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label Oct 6, 2021
@matomo-org matomo-org locked as resolved and limited conversation to collaborators Jan 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
c: Tests & QA For issues related to automated tests or making it easier to QA & test issues. Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Milestone
4.5.0
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@sgiehl @tsteur @Findus23 @flamisz @mattab @justinvelluppillai