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

Phpunit versions #732

Merged
merged 2 commits into from
Jun 11, 2023
Merged

Phpunit versions #732

merged 2 commits into from
Jun 11, 2023

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                                

> [email protected] 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