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

AB#112: Fixed a bug where Age returned 1 year less than expected. #113

Closed

Conversation

Pat-Riz
Copy link
Contributor

@Pat-Riz Pat-Riz commented Jan 9, 2025

Type of change

  • New feature
  • Bug fix
  • Security patch
  • Documentation update

Description

This fixes the Age property to return the correct age. See linked Issue for more details.

I have included a new test 'TestEdgeCasesAroundBirthday' that demonstrates the faulty behaviour with the previous implementation, and verifies the correct behaviour with the new implementation.

Note:
If we just change the Age implementation we will break all the tests for 'TestAge'.
However, It's my understanding that these tests are actually faulty and expects the wrong age. I.e the second testdata gets '200002292399' as input SSN and expects that person to be '25' years old.. that however is wrong since the 'TestTimeProvider' returns '2025-01-01'. This person has not yet had his birthday this year and therfor is 24 years old. All the inputdata for this test has the same issue.

I "solved" it by changing so that 'TestTimeProvider' returns 2025-10-05. Then all data for the tests has actually had their birthday, and it gives 'TestEdgeCasesAroundBirthday' an opportunity to check edgecases around the birthdays.

If I have misunderstood the previous tests or made wrong assumptions, let me know and I can try to fix it 😃

Related issue

#112

Motivation

To hopefully make things better 😄

Checklist

  • I have read the CONTRIBUTING document.
  • I have read and accepted the Code of conduct
  • Tests passes.
  • Style lints passes.
  • Documentation of new public methods exists.
  • New tests added which covers the added code.

@Johannestegner
Copy link
Member

Closing this PR (replaced by #115 which includes this commit).

Thank you for the contribution to the project! :)

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.

2 participants