Add unit tests for AddInstitutionProcessor to prepare invoicing for complete automation#295
Conversation
QuanMPhm
left a comment
QuanMPhm
left a comment
There was a problem hiding this comment.
Two comments, and can you squash the commits of the PR afterwards?
|
|
||
|
|
||
| class TestAddInstitutionProcessor(TestCase): | ||
| def _get_test_data(self, pi_names, projects=None, institutions=None): |
There was a problem hiding this comment.
Since you're always passing in projects, you might as well remove the default option from this function. Adding the default None here suggests the projects parameter was optionally in some of your tests, which is not the case
| assert pandas.isna(output.loc[0, "Manager (PI)"]) | ||
| assert output.loc[1, "Institution"] == "Boston University" |
There was a problem hiding this comment.
Why not do assert equal on the whole dataframe, like you did in the first test?
marcoagonzales007
force-pushed
the
add-institution-processor-tests
branch
from
4eefaa8 to
47faa22
Compare
part of CCI-MOC#185 Adds unit tests for AddInstitutionProcessor, bringing coverage from 60% to 100%. Tests added: test_add_institution — verifies that the Institution column is correctly populated based on each PI's email domain test_add_institution_missing_pi — verifies that rows where Manager (PI) is missing are skipped without raising an error Also adds new_add_institution_processor() to util.py to follow the existing pattern used by all other processors.
marcoagonzales007
force-pushed
the
add-institution-processor-tests
branch
from
47faa22 to
6f45e9d
Compare
| projects=["ProjectA", "ProjectB"], | ||
| institutions=["Boston University", "MIT"], | ||
| ) | ||
| answer_data = answer_data.astype(output.dtypes) |
There was a problem hiding this comment.
Interesting. We usually don't need to cast the type of the dataframe to pass the equals() assertion. Especially with just plain str dtypes. Did you encountered an error without the casting?
There was a problem hiding this comment.
I added it because I wasn't sure if the types would match during the comparison. I'll remove it and verify that the tests still pass without it.
2 participants
part of #185
Adds unit tests for
AddInstitutionProcessor, bringing coverage from 60% to 100%.Tests added:
test_add_institution— verifies that the Institution column is correctly populated based on each PI's email domaintest_add_institution_missing_pi— verifies that rows where Manager (PI) is missing are skipped without raising an errorAlso adds
new_add_institution_processor()toutil.pyto follow the existing pattern used by all other processors.