Skip to content

Phpunit versions#732

Merged
shivammathur merged 2 commits intoshivammathur:masterfrom
amnuts:phpunit-versions
Jun 11, 2023
Merged

Phpunit versions#732
shivammathur merged 2 commits intoshivammathur:masterfrom
amnuts:phpunit-versions

Conversation

@amnuts
Copy link
Contributor

@amnuts amnuts commented May 24, 2023


name: ⚙ Improvement: Make PHPUnit versions compatible with the PHP version installed
about: If you just let the phpunit version be latest (by not defining a specific version), then it can be incompatible with the version of php defined. This PR fixes that issue.
labels: enhancement


A Pull Request should be associated with a Discussion.

Related discussion: #694

Description

This PR makes an improvement to adding phpunit as a tool. If you just let the phpunit version be latest (by not defining a specific version), then it can be incompatible with the version of php defined. This PR adds version checking for phpunit and the php being setup.

  • I have written test cases for the changes in this pull request
  • I have run npm run format before the commit.
  • I have run npm run lint before the commit.
  • I have run npm run release before the commit.
  • npm test returns with no unit test errors and all code covered.

For those last two points:

The npm run release failed with the following error:

➜  setup-php git:(phpunit-versions) ✗ npm run release                                

> setup-php@2.25.1 release
> ncc build -o dist && git add -f dist/

Error: Cannot find module 'setup-php/lib/install.js'. Please verify that the package.json has a valid "main" entry

I could see no information in the CONTRIBUTION guide as to how to resolve that. I forced the change into the dist folder so that I could test it out in my own fork.

As to the tests: my tests passed just fine. But there were a couple outside of the scope of what I changed that failed - and failed before I even made any changes.

Here's a screenshot of it in practise. I cannot link to the action run as it's a private repo.

Screenshot 2023-05-24 at 13 53 45

@amnuts
Copy link
Contributor Author

amnuts commented May 30, 2023

@shivammathur, if you can throw me any advice on running the npm run release so it works for me, or what those failing tests should do, then I'm happy to try to make updates if required.

@codecov
Copy link

codecov bot commented Jun 11, 2023

Codecov Report

Merging #732 (b63f118) into master (cb8f453) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #732   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            6         6           
  Lines          549       558    +9     
  Branches        85        90    +5     
=========================================
+ Hits           549       558    +9     
Impacted Files Coverage Δ
src/tools.ts 100.00% <100.00%> (ø)

@shivammathur
Copy link
Owner

@amnuts

The error on npm run release is because it needs to be compiled first using npm run build. I have updated the CONTRIBUTING.md in 1b02c00.

@shivammathur shivammathur merged commit 66324ab into shivammathur:master Jun 11, 2023
@amnuts
Copy link
Contributor Author

amnuts commented Jun 17, 2023

Ahh, that's great @shivammathur, thanks!!

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