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

Revise test_reforms.py so that reform info is in a JSON file #1576

Merged
merged 1 commit into from
Oct 6, 2017
Merged

Revise test_reforms.py so that reform info is in a JSON file #1576

merged 1 commit into from
Oct 6, 2017

Conversation

martinholmer
Copy link
Collaborator

This pull request refactors the test_reforms.py code so that information about all of the 55 tested reforms is in one JSON file named reforms.json. These changes are based on the suggestion made by @hdoupe in an recent discussion of how to approach the reform tests.

These changes permit TaxBrain to specify an HTML link to the more readable reforms.json file, which contains no Python test code. There is no change in test logic and there is no change in tax-calculating logic or test results.

This pull request will require a change in one HTML link in the "Transparency and Replicability" section of the "About TaxBrain" page.

@MattHJensen @Amy-Xu @hdoupe @brittainhard

@codecov-io
Copy link

codecov-io commented Sep 29, 2017

Codecov Report

Merging #1576 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #1576   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files          37      37           
  Lines        2728    2728           
======================================
  Hits         2728    2728

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8976879...d6a8f72. Read the comment docs.

@MattHJensen
Copy link
Contributor

This is definitely an improvement. I could see this leading to a system where we write the results into reforms_actual and copy them to reforms_expect.json

@martinholmer
Copy link
Collaborator Author

@MattHJensen said about pull request #1576:

This is definitely an improvement. I could see this leading to a system where we write the results into reforms_actual and copy them to reforms_expect.json.

I didn't see how to do that because the 55 tests are not necessarily run in order (because of the use of py.test -n4 option). This makes writing an actual file impossible. Or, if not impossible, more work than it is worth.

@MattHJensen
Copy link
Contributor

@martinholmer said:

I didn't see how to do that because the 55 tests are not necessarily run in order (because of the use of py.test -n4 option). This makes writing an actual file impossible. Or, if not impossible, more work than it is worth.

I see, thanks for the explanation, @martinholmer

@hdoupe
Copy link
Collaborator

hdoupe commented Oct 4, 2017

Nice work @martinholmer. This looks good

@martinholmer
Copy link
Collaborator Author

Pull request #1576 has been open for seven days and discussion of it seems to be resolved, so it is being merged.

@martinholmer martinholmer merged commit 726d2b2 into PSLmodels:master Oct 6, 2017
@martinholmer martinholmer deleted the redo-test-reforms branch October 6, 2017 14:59
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.

4 participants