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

Run test from php 5.4 to 8.3, and fix issues with php8.2 of dependencies #512

Merged
merged 10 commits into from
Feb 17, 2024

Conversation

tenzap
Copy link
Collaborator

@tenzap tenzap commented Feb 13, 2024

This will:

  • ensure code style fixes/checks are always made with the same version for everyone and don't depend on the dependencies installed for kalkun
  • run tests from php 5.6 to the latest php version (list of released versions is dynamically fetched from php.net). BTW adding support for phpunit ranges from phpunit4 to phpunit10.
  • fix some code deprecations reported by php 8.1 & 8.2
  • add patches to add support for php 8.2 to dependencies (ci3 & kenjis/ci-phpunit-test)

set versions dynamically from currently really released php versions
and:
 - apply a workaround for a bug with phpunit <= 6

 - restore the Monkey patcher (which by default is commented out after having
   run install.php)

 - remove WelcomeTest.php

 - exlude the full application/view dir from coverage

 - fix void return type of setUp() with phpunit <= 6

     Should also be done for:

        PHPUnit\Framework\TestCase::setUpBeforeClass()
        PHPUnit\Framework\TestCase::setUp()
        PHPUnit\Framework\TestCase::assertPreConditions()
        PHPUnit\Framework\TestCase::assertPostConditions()
        PHPUnit\Framework\TestCase::tearDown()
        PHPUnit\Framework\TestCase::tearDownAfterClass()
        PHPUnit\Framework\TestCase::onNotSuccessfulTest()

See: https://phpunit.de/announcements/phpunit-7.html
Error was:

str_replace(): Passing null to parameter #1 () of type array|string is deprecated
because with vaimo/composer-patches:
- there is vaimo/composer-patches#115 which refrains
  from using it on php 7.0 7.1 & 7.2
- patching randomly fails with weird errors:

Processing patches configuration
  - Applying patches for codeigniter/framework (2)
    ~ kalkun-sms/kalkun: patches/Codeigniter_Framework/v3.1.13/10-php82_support.patch [NEW]
      Add support for PHP 8.2 (part1)
    ~ kalkun-sms/kalkun: patches/Codeigniter_Framework/v3.1.13/10-php82_support-part2.patch [NEW]
      Add support for PHP 8.2 (part2)

Error: Could not delete /home/runner/work/Kalkun/Kalkun/vendor/kenjis/ci-phpunit-test/CONTRIBUTING.md:

In Filesystem.php line 288:

  Could not delete /home/runner/work/Kalkun/Kalkun/vendor/kenjis/ci-phpunit-t
  est/CONTRIBUTING.md:

or

Processing patches configuration
  - Applying patches for codeigniter/framework (2)
    ~ kalkun-sms/kalkun: patches/Codeigniter_Framework/v3.1.13/10-php82_support.patch [NEW]
      Add support for PHP 8.2 (part1)
    ~ kalkun-sms/kalkun: patches/Codeigniter_Framework/v3.1.13/10-php82_support-part2.patch [NEW]
      Add support for PHP 8.2 (part2)

Error: Could not delete /home/runner/work/Kalkun/Kalkun/vendor/kenjis/ci-phpunit-test/.gitignore:

In Filesystem.php line 288:

  Could not delete /home/runner/work/Kalkun/Kalkun/vendor/kenjis/ci-phpunit-t
  est/.gitignore:
Creation of dynamic property MY_Loader:: is deprecated
@tenzap tenzap changed the title Run test on Run test from php 5.4 to 8.3, and fix issues with php8.2 of dependencies Feb 13, 2024
@tenzap tenzap mentioned this pull request Feb 13, 2024
Comment on lines +34 to +39
for minor in {0..20}; do
if dpkg --compare-versions ${major}.$minor le $upstream_ver && dpkg --compare-versions ${major}.$minor ge 5.6; then
PHP_VERSIONS+=("${major}.$minor")
fi
done
done
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there any other simpler way to identify the versions? Maybe we could have fixed values that we support? My only concern is that is is a version released which is not supported, suddenly the CI would stop working, and throwing failure, even when other versions are supported.

Copy link
Collaborator Author

@tenzap tenzap Feb 14, 2024

Choose a reason for hiding this comment

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

Is there any other simpler way to identify the versions?

I haven't found any other way using official input.

I prefer dynamic values, that's less maintenance burden and permits to identify when there is a problem with new releases.

About failing CI we could use something like this (mark some versions as non-experimental which would make fail, the other having continue_on_failure=true) ?
https://docs.github.com/en/actions/using-jobs/using-a-matrix-for-your-jobs#handling-failures

https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idcontinue-on-error

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, this seems better. Stable version shouldn't fail and would get rightly reported

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What do you mean by stable version? The list would only contain released versions of PHP, not betas or devel versions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Correct

Copy link
Collaborator

Choose a reason for hiding this comment

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

If we have only stable versions, then all good

Comment on lines +1 to +8
From fb1256a5b009b6264fbc85be44e0d97654d3fcd9 Mon Sep 17 00:00:00 2001
From: George Petculescu <gxgpet@gmail.com>
Date: Sun, 6 Nov 2022 16:13:43 +0200
Subject: [PATCH] Adding PHP 8.2 support

---
.github/workflows/test-phpunit.yml | 20 +++++++++++++++++++-
system/core/Loader.php | 1 +
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of having patches here, can we refer to pr's on the upstream?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

PR content might be changing in good or bad. I prefer this approach.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

BTW there is not always a PR. Here there is, but for ci-phpunit-test, there isn't.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Then maybe we should raise that to upstream?

Copy link
Collaborator Author

@tenzap tenzap Feb 15, 2024

Choose a reason for hiding this comment

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

:D it comes from upstream... bcit-ci/CodeIgniter#6173
Really I'm not in favor or using PRs, it may change and therefore it would be difficult to find a bug from someone who installs kalkun with a "PR patch", the content of which is evolving in time.
A patch in a file is the best way to stay reproducible.

Copy link
Collaborator Author

@tenzap tenzap Feb 15, 2024

Choose a reason for hiding this comment

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

oh you mean the other patch, bug is reported already. kenjis/ci-phpunit-test#413. I don't expect any change there unfortunately. Support seems to come to an end given the replies I get and the time that CI3 has been waiting for an update for recent PHP.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, okay for now, we should probably also upgrade to ci4 then given no support for ci3 anymore?

@tenzap
Copy link
Collaborator Author

tenzap commented Feb 17, 2024

Can I merge then?

@tenzap tenzap merged commit 786a087 into kalkun-sms:devel Feb 17, 2024
13 checks passed
@tenzap tenzap deleted the feature-tests branch February 17, 2024 20:58
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.

2 participants