-
-
Notifications
You must be signed in to change notification settings - Fork 197
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 New Zealand Holidays #207
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
$holidays = Holidays::for(country: 'nz')->get(); | ||
|
||
expect($holidays)->toBeArray()->not()->toBeEmpty(); | ||
expect(formatDates($holidays))->toMatchSnapshot(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of a complete snapshot test, it might be more interesting to only test the specific case (it has a substitute day for new year)
$holidays = Holidays::for(country: 'nz')->get(); | ||
|
||
expect($holidays)->toBeArray()->not()->toBeEmpty(); | ||
expect(formatDates($holidays))->toMatchSnapshot(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of a complete snapshot test, it might be more interesting to only test the specific case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took guidance from WalesTest.php as I wasn't quite sure what coverage would be appropriate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That test had been better with specific assertions too
Adding New Zealand holidays. Took guidance from Australia and Wales.