-
Notifications
You must be signed in to change notification settings - Fork 6
add to test coverage #243
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
add to test coverage #243
Conversation
copied implementations foudn in other navapbc repos
copied implementations found in other navapbc repos
copied implementations found in other navapbc repos
copied implementations found in other navapbc repos
@lisac thanks! for testing, could you make these same changes in a PR in https://github.com/navapbc/platform-test-flask so that we can make sure these changes passes CI? |
hi @lorenyu ! my apologies, this is my first time working in this repo. |
Co-authored-by: Michael Chouinard <46358556+chouinar@users.noreply.github.com>
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.
test_audit test is redundant and I don't think we should include the test_app_config test, everything else looks great, thanks!
assert_record_match(record, expected_record) | ||
|
||
|
||
def test_get_addr_info(init_audit_hook, caplog: pytest.LogCaptureFixture): |
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.
this test is redundant with test_audit_hook
, there's already a use case for socket.getaddrinfo (see
template-application-flask/template/{{app_name}}/tests/src/logging/test_audit.py
Lines 96 to 101 in 12c5d00
pytest.param( | |
socket.getaddrinfo, | |
("www.python.org", 80), | |
[{"msg": "socket.getaddrinfo", "audit.args.host": "www.python.org", "audit.args.port": 80}], | |
id="socket.getaddrinfo", | |
), |
|
||
|
||
@mock.patch.dict(os.environ, {"host": "192.123.123.123", "port": "5190"}) | ||
def test_app_config_sets_values_from_environment(): |
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'm on the fence about including this test and lean somewhat against including this test because it seems like it's kind of retesting pydantic's BaseSettings class. What sort of regression/bug is this test attempting to protect against?
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.
frankly, i was reacting to the test coverage % on that file. I agree with your point that it's retesting pydantic's implementation. have removed!
|
||
|
||
def test_join_list(): | ||
assert join_list(None) == "" |
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.
out of scope of this PR but why do we even have this function
already covered by a case in test_audit_hook()
Ticket
n/a
Changes
Copies over unit tests discovered in other repositories, where the subjects of the tests are functions made available by the template flask application.
Context for reviewers
Testing
navapbc/platform-test-flask#9