-
Notifications
You must be signed in to change notification settings - Fork 1
[NDH-324] Amendments to Fixtures and Test cases #256
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
base: main
Are you sure you want to change the base?
[NDH-324] Amendments to Fixtures and Test cases #256
Conversation
Signed-off-by: Isaac Milarsky <imilarsky@gmail.com>
Signed-off-by: Isaac Milarsky <imilarsky@gmail.com>
Signed-off-by: Isaac Milarsky <imilarsky@gmail.com>
Signed-off-by: Isaac Milarsky <imilarsky@gmail.com>
Backend Django Test Results98 tests ±0 96 ✅ - 2 1s ⏱️ ±0s For more details on these failures, see this check. Results for commit 81cd86c. ± Comparison against base commit 7aac220. ♻️ This comment has been updated with latest results. |
Signed-off-by: Isaac Milarsky <imilarsky@gmail.com>
Signed-off-by: Isaac Milarsky <imilarsky@gmail.com>
Signed-off-by: Isaac Milarsky <imilarsky@gmail.com>
Signed-off-by: Isaac Milarsky <imilarsky@gmail.com>
spopelka-dsac
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the broken address_sue test is specifically a problem with how this line is trying to identify whether an address use is present:
npd/backend/npdfhir/serializers.py
Line 85 in 3333620
| if "use" in representation.keys(): |
Can you please fix that line so that the test_list_filter_by_address_use test will pass?
The test_list_filter_by_npi_general failure is a result of the test needing to be updated, as I noted in my comment on the test.
| else: | ||
| ehr_vendor = ehr | ||
|
|
||
| et = EnvironmentType.objects.get(pk="prod") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need to get the associated object first? I think you should be able to just pass "prod" as the value in for environment_type in line 64. Same for the other areas where you're querying first
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get a ValueError when I try to just pass in a string
|
|
||
| def _ensure_endpoint_base_types(): | ||
| """ | ||
| Flyway inserts some reference values, but ensure minimal ones exist. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you say more about the behavior you were experiencing that necessitated these ensure functions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was a while ago but I think I did that because in the reference data sql file it inserts the endpoint_connection_type data in the reference data but it doesn't insert anything for the endpoint_type data. I wanted to make sure that it would handle if that data was added in the reference data sql file.
| last_name="ZOLLER", first_name="DAVID", practitioner_type=cls.nurse_code | ||
| last_name="ZOLLER", | ||
| first_name="DAVID", | ||
| practitioner_types=[cls.nurse_code, cls.non_nurse_code], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be helpful to create non-nurse practitioners as well, so we can ensure that there are some test practitioners in the test dataset that aren't nurses
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good I created a couple records for people that have other nucc classifications.
Signed-off-by: Isaac Milarsky <imilarsky@gmail.com>
Signed-off-by: Isaac Milarsky <imilarsky@gmail.com>
Signed-off-by: Isaac Milarsky <imilarsky@gmail.com>
Signed-off-by: Isaac Milarsky <imilarsky@gmail.com>
Signed-off-by: Isaac Milarsky <imilarsky@gmail.com>
Signed-off-by: Isaac Milarsky <imilarsky@gmail.com>
Signed-off-by: Isaac Milarsky <imilarsky@gmail.com>
Signed-off-by: Isaac Milarsky <imilarsky@gmail.com>
…ss when transformed into a string Signed-off-by: Isaac Milarsky <imilarsky@gmail.com>
Signed-off-by: Isaac Milarsky <imilarsky@gmail.com>
Signed-off-by: Isaac Milarsky <imilarsky@gmail.com>
Signed-off-by: Isaac Milarsky <imilarsky@gmail.com>
Signed-off-by: Isaac Milarsky <imilarsky@gmail.com>
Signed-off-by: Isaac Milarsky <imilarsky@gmail.com>
Amendments to Fixtures and Test cases
Jira Ticket #NDH-324
Problem
The current state of the amended test fixtures is not quite to specification for ticket NDH-324. Also, the fixtures file is getting to be very large and needs to be turned into a module of separate files.
Solution
Amend the test cases to check all returned data and make sure that we aren't getting back data that was not supposed to be fetched by the given filter parameter. Also, improve the tests by adding additional fixture data that adds a non NPI identifier. Additionally, improve the test cases by making sure all assertions match the filters.
I have also created a new fixtures module to replace the fixtures.py file because it was getting to be pretty large.
Result
Summary:
The amended tests have also exposed some issues in the code-base. Namely, the test that checks to see if the identifier filter can fetch non NPI identifiers does not find that identifier in the returned data. I think this is because the non NPI identifier is just not present in the serialized data although the correct record does appear to be fetched.
Additionally, the tests have also found that the 'use' field is missing from the returned practitioner data address field.
I also ran
make formatso there have been a bunch of formatting changes from ruff.