-
Notifications
You must be signed in to change notification settings - Fork 82
Make CountryTaxBenefitSystem reusable as a fixture #997
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
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 @maukoquiroga. This is a good forward step towards modularity!
I just have one general question regarding the 'simulation_builder' fixture (see inline comments).
Otherwise, generally looks good - happy to approve 😄
You're right! I've accepted your suggestion. |
Co-authored-by: Ram Parameswaran <[email protected]>
2465724
to
7311cea
Compare
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.
LGTM.
I have no additional concerns since my last review. Thanks for incorporating my comment about the simulation_builder fixture.
And the styleguide is fantastic 💥
Nice one @maukoquiroga! 👏
Related to #969 (it is almost a subset of it, it only extracts CountryTaxBenefitSystem to a fixture).
Related to #984 (it adds more modularity but does not evince country template's scenarios).
Technical changes
CountryTaxBenefitSystem
to a fixture reusable by all the tests in the test suite.Performance
Impact on performance of the test suite is neglectable.
Before:
After: