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

Test specific provider #86

Merged
merged 4 commits into from
Dec 29, 2021
Merged

Test specific provider #86

merged 4 commits into from
Dec 29, 2021

Conversation

willpower232
Copy link
Collaborator

@willpower232 willpower232 commented Dec 19, 2021

As this library supports two third party QR code generators "out of the box", it is a good idea to test them but separately as the composer dependencies could get a bit messed up.

This PR lays out a mechanism for doing so (and fixes a little problem as well).

@willpower232 willpower232 self-assigned this Dec 19, 2021
@willpower232 willpower232 force-pushed the test-specific-provider branch from 7993e0e to cb45226 Compare December 19, 2021 19:50
@willpower232 willpower232 marked this pull request as ready for review December 19, 2021 19:53

- uses: ramsey/composer-install@v1

- run: composer require bacon/bacon-qr-code
Copy link
Contributor

@MasterOdin MasterOdin Dec 19, 2021

Choose a reason for hiding this comment

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

Why not just make it a dev dependency? Same with the other third-party libraries? They're necessary to run the full test suite, and would avoid having an independent workflow across all supported PHP versions for this one (or several) external dependencies? Within the dev dependencies, you then can also specify the range you wish to test against (e.g. "bacon/bacon-qr-code": "^1 | ^2") and then within the composer install line pass --prefer-lowest when testing against PHP 5.6, where ^1 is only to test that the exception throws.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

in reality, the end user would only install bacon or endroid so the tests should recreate that environment

also adding them both as development dependencies could get complicated as endroid relies on bacon

@RobThree
Copy link
Owner

I'll be having vacation the week between christmas and new year's eve. I'm hoping to get around to this and really be able to take a few minutes and sit down and have a decent look at this. I'm sorry for the lack of response.

@willpower232
Copy link
Collaborator Author

no worries @RobThree I wanted to get some progress on the PHP 8.1 thing and this issue and finally had some moments and a working laptop to do it in 😅

@RobThree
Copy link
Owner

From what I see and understand, this looks good to me! I'll leave the honour of pressing the "Merge Pull Request" button to you @willpower232! Great work (as usual), thank you!

@willpower232 willpower232 merged commit 5d36d4f into master Dec 29, 2021
@willpower232 willpower232 deleted the test-specific-provider branch December 29, 2021 18:18
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.

3 participants