Add tests for warning_messages.py file
#62
Conversation
facebook-github-bot
added
the
CLA Signed
|
@0xedward has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
There was a problem hiding this comment.
Thanks for working on this! :) Sorry about the delay getting to this
Would you be able to fix the type checking errors?
sapp/tests/warning_messages_test.py:36:4 Missing return annotation [3]: Returning `None` but no return type is specified.
sapp/tests/warning_messages_test.py:40:16 Undefined attribute [16]: Optional type has no attribute `message`.
sapp/tests/warning_messages_test.py:54:4 Missing return annotation [3]: Returning `None` but no return type is specified.
sapp/tests/warning_messages_test.py:63:69 Incompatible parameter type [6]: Expected `int` for 3rd positional only parameter to call `upsert_entry` but got `str`.
|
@0xedward has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
@0xedward has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
sapp/tests/warning_messages_test.py
Outdated
| temp = tempfile.NamedTemporaryFile(mode="w+") | ||
| json.dump(test_metadata, temp) | ||
| temp.flush() |
There was a problem hiding this comment.
Would we be able to use a context manager to manage closing the file and the db session here?
Since we will have nested with statements here, we might want to use contextlib.ExitStack
For example, the following pseudocode:
with ExitStack() as stack:
temp_file = stack.enter_context(tempfile.NamedTemporaryFile(mode="w+")
json.dump(test_metadata, temp_file)
...
session = stack.enter_context(self.db.make_session())
try:
...
code1001 = (
session.query(WarningMessage).filter_by(code="1001").one_or_none()
)
...
arpancodes
force-pushed
the
warning-messages-test
branch
from
ea05063 to
df8ab6c
Compare
|
@0xedward has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
sapp/tests/warning_messages_test.py
Outdated
| with ExitStack() as stack: | ||
| temp = stack.enter_context(tempfile.NamedTemporaryFile(mode="w+")) | ||
| json.dump(test_metadata, temp) | ||
| temp.read() |
There was a problem hiding this comment.
What's the point of the read here, if we are not using the result?
There was a problem hiding this comment.
If we don't call .read(), we are not able to use the file content of it later on the line number 41
There was a problem hiding this comment.
It looks like temp.read() reads the contents we dumped with json.dump(test_metadata, temp). Without the call, temp will be empty when we call path.Pathlib.read_text(temp.name).
It looks like we might be able to avoid calling temp.read() if we set the mode during instantiation of temp to 'w+b', but then we cannot write a string to temp, we would have to encode test_metadata as byte object to be able to write it to temp. Would you prefer if we approach it this way instead @arthaud?
There was a problem hiding this comment.
I honestly still don't understand why we would need that .read().
My only idea is that .read() flushes some buffer and the next read actually works? What if we use temp.flush() instead?
Also, not sure w+b will change anything.
There was a problem hiding this comment.
temp.flush() works as well, I can change it to that 😄
|
@0xedward has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
Pre-submission checklist
black .usort format .flake8pip install -r requirements-dev.txtand completed the following:./scripts/run-tests.shand made sure all tests are passingSummary
Added tests for the file
/sapp/warning_messages.pyTest Plan
Relevant issue: MLH-Fellowship#11