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

Add test for eitc rate greater than 1 #674

Closed
wants to merge 2 commits into from

Conversation

hdoupe
Copy link
Collaborator

@hdoupe hdoupe commented Oct 2, 2017

This test exposes the bug found in issue #596

@martinholmer
Copy link
Contributor

@hdoupe, Why are you adding a TaxBrain test to detect a perfectly valid parameter value?
Look at taxcalc/current_law_policy.json to see that eitc_rt can have any non-negative value.
What am I missing? I don't see why this TaxBrain test is needed. It seems wrong, not just unneeded.

@hdoupe
Copy link
Collaborator Author

hdoupe commented Oct 2, 2017

@martinholmer I added this as a regression test. I wanted to make sure that we didn't have any issues in the future with EITC_rt parameter values or values for similar parameters being rounded down to 1.

Does this clear up the confusion?

@martinholmer
Copy link
Contributor

Oh, the test is to see if values of _EITC_rt greater than one are allowed.
OK, I missed that. So, adding the test seems fine.

But I don't understand the new test's docstring:

    def test_taxbrain_eitc_rt_greater_than_1(self):
        """
        Transfer over the regular tax capital gains to AMT
        """

@hdoupe
Copy link
Collaborator Author

hdoupe commented Oct 2, 2017

@martinholmer Thanks for pointing that out. I copied and pasted from another function and forgot to change the docstring. The latest commit updates the docstring.

@martinholmer
Copy link
Contributor

@hdoupe, #674 looks good now.

@hdoupe
Copy link
Collaborator Author

hdoupe commented Nov 8, 2017

#674 test is covered in PR #706

@hdoupe hdoupe closed this Nov 8, 2017
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