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

Fix Person.birthDate range error during leap year #205

Conversation

JoaoSouMoreira
Copy link
Contributor

@JoaoSouMoreira JoaoSouMoreira commented Jan 3, 2024

Hello, first of all thank you for creating this incredibly useful library.

The company I work at uses this library extensive in its automated testing, and during an incident on Dec 31st 2023 we started seeing randomly failing tests with the following error:

java.util.NoSuchElementException: Cannot get random in empty range: 366..365
	at kotlin.ranges.RangesKt___RangesKt.random(_Ranges.kt:193)
	at io.github.serpro69.kfaker.provider.Person.birthDate(Person.kt:38)
	at io.github.serpro69.kfaker.provider.Person.birthDate$default(Person.kt:34)
	at io.github.serpro69.kfaker.provider.PersonTest$1$1$1$2.invokeSuspend(PersonTest.kt:25)

After looking through the birthDate method's code and seeing the hard coded 365 I immediately assumed it would have been related to leap years, though I am still confused as to why Java recognizes LocalDate.now().isLeapYear as true on Dec 31st 2023. I think this change will fix the issue regardless.

Reproduction steps:

  • Change your device's date to December 31st 2023 (time does not matter)
  • Run all tests in PersonTest.kt
  • See test birthDate(age) fun failing with the error outlined above.

@serpro69 serpro69 merged commit 7c49653 into serpro69:master Jan 3, 2024
1 check passed
@serpro69
Copy link
Owner

serpro69 commented Jan 3, 2024

Hi @JoaoSouMoreira ,
Thanks a lot for this fix! Working with dates is always fun :D I have a suspicion why this could fail on 31st Dec particularly, but I'd need to look closer at it to know for sure.
I do want to add some special tests for such corner cases, but I'm not really sure of a good way to do that (in this particular case one would need to set JVM date/time to a particular value and run the given test on those settings)
Regardless, the change looks good! Thanks again. Merged.

PS: I'll make an RC version with the fix included in a day or two, in the meantime please use the snapshot if it's possible for you, which should be available when the build on master is finished

@JoaoSouMoreira
Copy link
Contributor Author

Hi @JoaoSouMoreira , Thanks a lot for this fix! Working with dates is always fun :D I have a suspicion why this could fail on 31st Dec particularly, but I'd need to look closer at it to know for sure. I do want to add some special tests for such corner cases, but I'm not really sure of a good way to do that (in this particular case one would need to set JVM date/time to a particular value and run the given test on those settings) Regardless, the change looks good! Thanks again. Merged.

PS: I'll make an RC version with the fix included in a day or two, in the meantime please use the snapshot if it's possible for you, which should be available when the build on master is finished

Thank you for taking a look at this so quickly! Yeah I was considering adding a specific test for this but it would require either changing core functionality to include the use of a Singleton Clock or installing a library like Mockito or Mockk and set the test up with mocks for LocalDate methods.
Either one of these options can be a follow-up for sure!

No rush on getting an RC version out! Thankfully we are quite a few days away from the 31st of December 😆

@serpro69 serpro69 added this to the 1.16.0 milestone Feb 11, 2024
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