Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Rename plus and minus to add and sub #282

Merged
merged 5 commits into from
Aug 11, 2022
Merged

Conversation

christian-kolb
Copy link
Contributor

Change Log

Changed

  • Changed all methods from `plus` to `add` and `minus` to `sub`.

Description

Resolves #281

@christian-kolb
Copy link
Contributor Author

christian-kolb commented Aug 11, 2022

@norberttech When running the tests locally with a PHP version that has the date.timezone UTC configured (I think that's the cause), the tests are randomly failing.

For example:

There was 1 failure:

1) Aeon\Calendar\Tests\Unit\Gregorian\DateTimeTest::test_creating_datetime_from_string with data set #25 ('2020-10-25 02:30:00+02:00', '2020-10-25 02:30:00 Europe/Warsaw', 'Y-m-d H:i:sP')
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'2020-10-25 02:30:00+02:00'
+'2020-10-25 02:30:00+01:00'
There was 1 failure:

1) Aeon\Calendar\Tests\Unit\Gregorian\DateTimeTest::test_creating_datetime_from_string with data set #25 ('2020-10-25 02:30:00+02:00', '2020-10-25 02:30:00 Europe/Warsaw', 'Y-m-d H:i:sP')
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'2020-10-25 02:30:00+02:00'
+'2020-10-25 02:30:00+01:00'

I don't think that it's related to the PR but something that should be looked into 🙂

@norberttech
Copy link
Member

Thanks, this looks great!
Before I merge this one, let me first try to figure out what's wrong with those failing tests since local timezone setting should not affect this test 🤔

@christian-kolb
Copy link
Contributor Author

Thanks, this looks great! Before I merge this one, let me first try to figure out what's wrong with those failing tests since local timezone setting should not affect this test 🤔

As they're also failing on the CI, I guess it's more because of a summer time / winter time switch where the tests aren't taking this into account. It might be better to create the dates in the timezone UTC. Otherwise the tests need to account for that change explicitly 🙂

@norberttech
Copy link
Member

Unfortunately, I'm afraid it's another BC break in PHP, unrelated to the UTC in php.ini settings, check here: #283

It would not be the first one, https://github.com/aeon-php/rate-limiter is broken due to a bug in how PHP started to handle microseconds in DateInterval, it was already fixed but it looks like it will be released in 8.1.10

@christian-kolb
Copy link
Contributor Author

So the current 8.1 release has an issue with handling DateIntervals? Good to know.
Only the mutation tests are still failing.

@norberttech
Copy link
Member

Yeah, in 8.1 definitely rate limiter is affected but if you don't need bigger precision than 1s you should be fine.
Hmm those mutation tests are failing because you added new methods and old ones are now not covered which means MSI score is not 100% anymore.

Could you maybe try to use @infection-ignore-all above those all methods to see if this will make it happy? Another way around would be to keep in tests old methods as they internally use new ones and switch while migrating to 2.0

@christian-kolb
Copy link
Contributor Author

I've updated it, but don't know what information I can get out of the report. Can you help me here?

@norberttech
Copy link
Member

Yeah, I see it didn't help.
In general, if you never used infection I recommend you to go through their readme but in this case, you can also simply reduce the MSI score in the infection.json file to 99 so I can merge it and tackle those mutants later as they are harmless in this case.

@christian-kolb
Copy link
Contributor Author

I would like to dive deeper into the topic first. So for now I reduced the score to 99. Thanks for your help 🙂

@norberttech norberttech merged commit a85c6fd into aeon-php:1.x Aug 11, 2022
@norberttech
Copy link
Member

Thanks @christian-kolb this was truly needed contribution!
I will tag the new release once I resolve this MSI issue

@christian-kolb
Copy link
Contributor Author

Awesome, thanks for your help 🙂

@norberttech
Copy link
Member

@christian-kolb
Copy link
Contributor Author

@norberttech That was quick, thanks a lot for your help on this 🙂

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Different naming for mutation methods
2 participants