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

add(test) enable phpunit tests #295

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

Conversation

connorhu
Copy link
Collaborator

@connorhu connorhu commented Dec 16, 2023

and its done.

PR adds phpunit. Unit tests are removed at last and then I will add the finished work bit by bit. I decided to do this because the work was becoming unmanageably large. I know why I did what I did, but for you to see through and approve it, it's easier to go component by component.

this depends on #325

@connorhu connorhu changed the title port tests to use phpunit [test] port tests to use phpunit Dec 16, 2023
@connorhu connorhu force-pushed the feature/phpunit branch 18 times, most recently from 132670f to 177b6d0 Compare December 20, 2023 16:06
@connorhu connorhu changed the title [test] port tests to use phpunit add(test) enable phpunit tests Jan 14, 2024
@connorhu connorhu force-pushed the feature/phpunit branch 5 times, most recently from 10c20e6 to ec909a0 Compare January 14, 2024 12:22
@connorhu connorhu requested review from thePanz and thirsch January 14, 2024 12:30
phpunit.xml.dist Outdated Show resolved Hide resolved
@connorhu connorhu changed the base branch from master to 1.6.x January 19, 2024 12:26
@connorhu connorhu force-pushed the feature/phpunit branch 2 times, most recently from 94a1d14 to 356960a Compare January 19, 2024 12:34
test/bin/test Outdated Show resolved Hide resolved
@connorhu connorhu force-pushed the feature/phpunit branch 3 times, most recently from f27893f to bbcc676 Compare February 16, 2024 06:48
@thePanz
Copy link
Member

thePanz commented Feb 20, 2024

@connorhu would you extract the removal of the Version tag into its own PR?
This way we can have a more focused (and reduced) changeset 👍

@connorhu
Copy link
Collaborator Author

@connorhu would you extract the removal of the Version tag into its own PR? This way we can have a more focused (and reduced) changeset 👍

Sorry, I don't know how it got there.

@connorhu connorhu changed the base branch from 1.6.x to master February 20, 2024 12:46
@connorhu connorhu force-pushed the feature/phpunit branch 5 times, most recently from 8b393e8 to 0b27f22 Compare February 20, 2024 12:59
@connorhu
Copy link
Collaborator Author

php-cs-fixer 403: rate limit exceeded ☹️

@connorhu
Copy link
Collaborator Author

I studied some how-tos and changed my mind. For phpunit testing we will also prefer phar based installs. The composer/script section shows which version is preferred. This can be expanded later if we want to support higher phpunit versions. For now I don't see the sense in this, version 9 will be fine.

@thePanz
Copy link
Member

thePanz commented Feb 24, 2024

I studied some how-tos and changed my mind. For phpunit testing we will also prefer phar based installs. The composer/script section shows which version is preferred. This can be expanded later if we want to support higher phpunit versions. For now I don't see the sense in this, version 9 will be fine.

I would suggest to use phive to install the phar files. We could just ship the '.phive/phive.xml' file in the repo, where versioning is done there.

Drawback: the phive will run the installation without checking the currently installed PHP version, we would need to tale some extra care of the versioning, or have 2 phive.xml for php7 and php8

Wdyt?

@connorhu
Copy link
Collaborator Author

connorhu commented Feb 24, 2024

I would suggest to use phive to install the phar files. We could just ship the '.phive/phive.xml' file in the repo, where versioning is done there.

Drawback: the phive will run the installation without checking the currently installed PHP version, we would need to tale some extra care of the versioning, or have 2 phive.xml for php7 and php8

Wdyt?

I'm going to dig into this and figure out what we can do here with phive.

@connorhu
Copy link
Collaborator Author

I have some concerns about this tool. It can do some things but it's a bit awkward to use.

  • phars xml undocumented, no xsd
  • writes the installed version into the config and you can't make it not do that
  • you need a phars.dist.xml file which has no exact version. for us it's enough to say that we need phpunit ^9.6, the detailed version is not interesting
  • you can't set an alternative configuration file for it

We can live with these gaps, but it's a bit meh.

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

Successfully merging this pull request may close these issues.

3 participants