Create Argentinian holidays provider. #264
Conversation
Update
ScreenShots(This is a system we use for work logging, we pull the holidays into the work calendar) Next
TB3h |
There was a problem hiding this comment.
@nedSaf Checked all the days. They look good. The timezone is incorrect. Santiago is located in Chile.
|
Thanks @nedSaf for your work! Nice to have a another country added. Please do not forget to add unit tests. |
nedSaf
changed the title
Update
ScreenShotsUnit test result
Holiday names in English
Review@stelgenhof This is ready for your review. |
|
Fixing tests... |
There was a problem hiding this comment.
@nedSaf looks good. There are two methods that could be renamed in the test.
Co-authored-by: Mariano D'Agostino <mariano-dagostino@users.noreply.github.com>
Co-authored-by: Mariano D'Agostino <mariano-dagostino@users.noreply.github.com>
|
@nedSaf You may want to check your tests as some of them are failing. |
|
@stelgenhof I fixed all the tests, ready for your review :) |
tests/Argentina/ArgentinaTest.php
Outdated
| */ | ||
| public function testOfficialHolidays(): void | ||
| { | ||
| $this->assertDefinedHolidays([ |
There was a problem hiding this comment.
This asserts that these holidays all are defined from the start test year (1980), which is correct. However it doesn't assert that the respective holidays are not present before their establishment year.
There was a problem hiding this comment.
Each test file has a method called testNotHoliday, which does exactly that (I know that they run for sure because they failed and I had to fix them), this structure of testing I copied from other providers so I think for consistency we should keep it as is.
But if you think otherwise, please let me know how you would like me to change the structure of the files?
There was a problem hiding this comment.
Just ran your test again and it fails:
1) Yasumi\tests\Argentina\ArgentinaTest::testObservedHolidays
Failed asserting that an array has the key 'carnavalMonday'.
You can replicate this by setting the starting test year lower than the establishment year. The error might not advertise itself as the actual test year is randomized.
There was a problem hiding this comment.
@stelgenhof I'm not sure what are you asking me to do here.
Each test has its own ESTABLISHMENT_YEAR on which the whole test is based, this is what I found in the other providers.
you can replicate this by setting the starting test year lower than the establishment year.
Yes, but it's set to a specific year on setUp, it will not fail unless you change the year.
The tests will fail if you change anything in the code, in this state, it will not fail.
In any case, please let me know exactly what you want me to do if you think I'm wrong, I'm not sure what I need to do.
There was a problem hiding this comment.
See for example this: https://github.com/azuyalabs/yasumi/blob/develop/tests/Georgia/GeorgiaTest.php#L65..L67
Your tests will not fail as your initial test start year (1980) is after any of the establishment years. It possibly can fail if you instantiate Yasumi with a year before any of the establishment years. Agreeably users might not do that a lot, but still is possible in case you'd like to see a backdated list of holidays.
|
@stelgenhof Fixed your comments, with one comment from me ^^ |
|
Pushed a fix. |
|
@nedSaf Did you have a chance to look at my last comment? Thanks! |
|
@stelgenhof Hey, I did push a fix after your comment, 233f9d4 I think that will cover the case you mentioned before, if not, please let me know. |
|
@stelgenhof any updates? |
No description provided.