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

Implement parameter validation for input_fields #907

Merged
merged 6 commits into from
Jun 21, 2018

Conversation

lucassz
Copy link
Contributor

@lucassz lucassz commented Jun 18, 2018

Implement parameter validation for input_fields, both for TaxBrain and for the partial equilibrium model.

@hdoupe
Copy link
Collaborator

hdoupe commented Jun 18, 2018

@lucassz This looks good. Can you add a test demonstrating the validation behavior? There aren't any specific tests for the forms. You can create a new file taxbrain/tests/test_forms.py with the new tests. A test with OK parameters and a test with unknown parameters should be sufficient.

You can look to webapp/apps/test_assets/test_models.py and webapp/apps/taxbrain/tests/test_models.py for some relevant examples.

@lucassz
Copy link
Contributor Author

lucassz commented Jun 18, 2018

@hdoupe Done. It seemed more relevant to add it to test_views.py since there was similar code there already.

@@ -57,6 +57,20 @@ def test_taxbrain_post(self, data_source):
check_posted_params(result['tb_dropq_compute'], truth_mods,
str(START_YEAR), data_source=data_source)

@pytest.mark.xfail
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lucassz This shouldn't be xfail-ed. That means that py.test will ignore it if the test fails.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hdoupe Thank you. I think I had understood the purpose of xfail.

@hdoupe
Copy link
Collaborator

hdoupe commented Jun 20, 2018

@lucassz This looks great. Thanks for taking care of this and for finding a much simpler way to solve the problem.

@hdoupe
Copy link
Collaborator

hdoupe commented Jun 20, 2018

@lucassz This should be good to go once you knock out the merge conflicts.

@lucassz
Copy link
Contributor Author

lucassz commented Jun 21, 2018

@hdoupe Happy to do it. Should be good to go now.

@hdoupe
Copy link
Collaborator

hdoupe commented Jun 21, 2018

Great, thanks @lucassz

@hdoupe hdoupe merged commit 1c9df5a into ospc-org:master Jun 21, 2018
@lucassz lucassz deleted the paramval branch June 21, 2018 13:50
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