140 validation metadata #165
Conversation
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||||||||
| val_res["logic_warnings"]["count"], val_res["logic_errors"]["count"] = len( | ||
| val_res["logic_warnings"]["details"] | ||
| ), len(val_res["logic_errors"]["details"]) |
There was a problem hiding this comment.
using tuple destructuring doesn't add much value here... you save 1 keystroke of not having to type out an additional =; but the formatting makes it a little more complicated to read.
| val_res["syntax_errors"] = {"count": 0, "details": []} | ||
| val_res["syntax_errors"]["details"] = val_json | ||
| val_res["syntax_errors"]["count"] = len(val_res["syntax_errors"]["details"]) |
There was a problem hiding this comment.
these can just be assigned directly instead of having an inititializing step.
|
let's also expand the test coverage on this PR, I don't think all the scenarios are covered in the existing tests; which is probably why the overall coverage decreased, and the PR coverage is only 26%; a few test scenarios would be to have the validation results return syntactical phase, logical phase; logical phase with both errors and warnings, logical with only errors, and logical with only warnings. |
tests/services/conftest.py
Outdated
| @pytest.fixture(scope="function") | ||
| def df_to_json_mock(mocker: MockerFixture, validate_submission_mock: Mock): | ||
| mock_json_formatting = mocker.patch("sbl_filing_api.services.submission_processor.df_to_json") | ||
| mock_json_formatting.return_value = "[{}]" | ||
| def validation_success_mock(mocker: MockerFixture, validate_submission_mock: Mock): | ||
| mock_json_formatting = mocker.patch("sbl_filing_api.services.submission_processor.build_validation_results") | ||
| mock_json_formatting.return_value = """ | ||
| { | ||
| "syntax_errors": { | ||
| "count": 0, | ||
| "details": [] | ||
| }, | ||
| "logic_errors": { | ||
| "count": 0, | ||
| "details": [] | ||
| }, | ||
| "logic_warnings": { | ||
| "count": 0, | ||
| "details": [] | ||
| } | ||
| }""" | ||
| return mock_json_formatting | ||
|
|
||
|
|
||
| @pytest.fixture(scope="function") | ||
| def validation_syntax_errors_mock(mocker: MockerFixture, validate_submission_mock: Mock): | ||
| mock_json_formatting = mocker.patch("sbl_filing_api.services.submission_processor.build_validation_results") | ||
| mock_json_formatting.return_value = """ | ||
| { | ||
| "syntax_errors": { | ||
| "count": 2, | ||
| "details": [] | ||
| } | ||
| }""" | ||
| return mock_json_formatting | ||
|
|
||
|
|
||
| @pytest.fixture(scope="function") | ||
| def validation_logic_warnings_mock(mocker: MockerFixture, validate_submission_mock: Mock): | ||
| mock_json_formatting = mocker.patch("sbl_filing_api.services.submission_processor.build_validation_results") | ||
| mock_json_formatting.return_value = """ | ||
| { | ||
| "syntax_errors": { | ||
| "count": 0, | ||
| "details": [] | ||
| }, | ||
| "logic_errors": { | ||
| "count": 0, | ||
| "details": [] | ||
| }, | ||
| "logic_warnings": { | ||
| "count": 1, | ||
| "details": [] | ||
| } | ||
| }""" | ||
| return mock_json_formatting | ||
|
|
||
|
|
||
| @pytest.fixture(scope="function") | ||
| def validation_logic_errors_mock(mocker: MockerFixture, validate_submission_mock: Mock): | ||
| mock_json_formatting = mocker.patch("sbl_filing_api.services.submission_processor.build_validation_results") | ||
| mock_json_formatting.return_value = """ | ||
| { | ||
| "syntax_errors": { | ||
| "count": 0, | ||
| "details": [] | ||
| }, | ||
| "logic_errors": { | ||
| "count": 4, | ||
| "details": [] | ||
| }, | ||
| "logic_warnings": { | ||
| "count": 0, | ||
| "details": [] | ||
| } | ||
| }""" | ||
| return mock_json_formatting | ||
|
|
||
|
|
||
| @pytest.fixture(scope="function") | ||
| def validation_logic_warnings_and_errors_mock(mocker: MockerFixture, validate_submission_mock: Mock): | ||
| mock_json_formatting = mocker.patch("sbl_filing_api.services.submission_processor.build_validation_results") | ||
| mock_json_formatting.return_value = """ | ||
| { | ||
| "syntax_errors": { | ||
| "count": 0, | ||
| "details": [] | ||
| }, | ||
| "logic_errors": { | ||
| "count": 3, | ||
| "details": [] | ||
| }, | ||
| "logic_warnings": { | ||
| "count": 2, | ||
| "details": [] | ||
| } | ||
| }""" | ||
| return mock_json_formatting |
There was a problem hiding this comment.
mocking out the return value for the method you are trying to test isn't productive; you are basically bypassing testing method, which is why the test coverage remains unchanged; if you click on the link with the lines in the test coverage comment, it shows the whole method is uncovered.
There was a problem hiding this comment.
ideally, you would be constructing the dataframe to pass to the method for testing, but if that's too much of a lift, something you can mock for example could be df_to_json()
| val_res["logic_warnings"]["count"] = len(val_res["logic_warnings"]["details"]) | ||
| val_res["logic_errors"]["count"] = len(val_res["logic_errors"]["details"]) | ||
|
|
||
| return val_res |
There was a problem hiding this comment.
This else block can be even further refined to:
errors_list = [e for e in val_json if e["validation"]["severity"] == Severity.ERROR.value]
warnings_list = [e for e in val_json if e["validation"]["severity"] == Severity.WARNING.value]
val_res = {
"syntax_errors": {"count": 0, "details": []},
"logic_errors": {"count": len(errors_list), "details": errors_list},
"logic_warnings": {"count": len(warnings_list), "details": warnings_list},
}
to avoid looping and appending. List comprehension in python is faster on large data sets.
| if result[2] == ValidationPhase.SYNTACTICAL.value: | ||
| val_res = {"syntax_errors": {"count": 0, "details": []}} | ||
| val_res["syntax_errors"]["details"] = val_json | ||
| val_res["syntax_errors"]["count"] = len(val_res["syntax_errors"]["details"]) |
There was a problem hiding this comment.
Just like below, this section can be shortened to just:
val_res = {"syntax_errors": {"count": len(val_json), "details": val_json}}
Closes #140