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

[TESTS] Running in CI should not create but rather fail on missing snapshots #1039

Merged
merged 3 commits into from
Sep 7, 2020

Conversation

mfn
Copy link
Collaborator

@mfn mfn commented Sep 6, 2020

Summary

Otherwise they get created on the CI infrastructure but that's it, the
build won't fail although it was forgotten to add the snapshot.

Note: I had specify a minimum phpunit version, otherwise phpunit 8.0.0 would be installed for Laravel 6 and prefer-lowest, which has the following bug with the -d parameter:

PHP Fatal error:  Uncaught TypeError: ini_set() expects parameter 2 to be string, bool given in /home/runner/work/laravel-ide-helper/laravel-ide-helper/vendor/phpunit/phpunit/src/TextUI/Command.php:379

…pshots

Otherwise they get created on the CI infrastructure but that's it, the
build won't fail although it was forgotten to add the snapshot.
@mfn mfn self-assigned this Sep 6, 2020
The `prefer-lowest` version we get with L6 is 8.0.0 which throws this error:
```
PHP Fatal error:  Uncaught TypeError: ini_set() expects parameter 2 to be string, bool given in /home/runner/work/laravel-ide-helper/laravel-ide-helper/vendor/phpunit/phpunit/src/TextUI/Command.php:379
```
@mfn mfn marked this pull request as ready for review September 6, 2020 20:52
@mfn mfn requested a review from barryvdh September 6, 2020 20:52
@barryvdh
Copy link
Owner

barryvdh commented Sep 7, 2020

I made the phpunit version more explicit. Looks good otherwise

@barryvdh barryvdh merged commit affa551 into barryvdh:master Sep 7, 2020
@mfn mfn deleted the mfn-tests-no-create-snapshot branch September 7, 2020 07:47
@mfn
Copy link
Collaborator Author

mfn commented Sep 7, 2020

I made the phpunit version more explicit. Looks good otherwise

Btw, I decided not to do that because the phpunit version is effectively dicdated by orchestra/testbench and the only goal was to make sure we don't use known broken version for our tests.

Otherwise once we bump orchestra which supports phpunit 10 we might stay on 9 for no reason but this constraint.

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.

None yet

2 participants