Add txt_op unit test #177
Add txt_op unit test #177
Conversation
SayaZhang
commented
Feb 15, 2024
SayaZhang
commented
- add txt_op unit test
- update tests file directory
SayaZhang
requested review from
CambioML,
goldmermaid,
CluckRookie and
SeisSerenata
as code owners
tests/op/extract/load/test_txt_op.py
Outdated
| @@ -0,0 +1,89 @@ | |||
| import unittest | |||
|
|
|||
| from moto import mock_s3 | |||
There was a problem hiding this comment.
It looks like the build is failing due to moto is not in pyproject.toml as a dependency.
I never used moto instead unittest mock. Personally, I would prefer to use mock from unittest https://docs.python.org/3/library/unittest.mock.html. But, let's discuss and maybe leave it to future tasks.
Based on my lower comment, after you delete, test_load_from_s3 and remove from moto import. You unittest should pass build.
There was a problem hiding this comment.
Okay, I deleted test_load_from_s3 and removed import moto. The test_load_from_s3 should be added in read_file unit test file.
tests/op/extract/load/test_txt_op.py
Outdated
| # arrange | ||
| node = Node( | ||
| name="node1", | ||
| value_dict={"filename": "tests/op/extract/data/empty.txt"}, |
There was a problem hiding this comment.
qq: this looks like a your must run the unittest in the root folder. However, people might run unittest in the sub load folder which might cause problem for this unittest.
Usually, I will not test I/O as well as external function call such as read_file in extract_txt_op. Instead, I would mock it with return_value to test method specific logic.
There was a problem hiding this comment.
Okay, I changed reading local file to mock it with return_value.
Does read_file need to test I/O by reading the local file and s3 file?
tests/op/extract/load/test_txt_op.py
Outdated
| conn = boto3.resource("s3", region_name="us-west-2") | ||
| conn.create_bucket(Bucket="uniflow-test") | ||
| s3 = boto3.client("s3", region_name="us-west-2") | ||
| s3.put_object(Bucket="uniflow-test", Key="test.txt", Body="mycontent") |
There was a problem hiding this comment.
qq: is this mocking a file is put in s3?
tests/op/extract/load/test_txt_op.py
Outdated
| ) | ||
| def test_call(self, mock_read_file): | ||
| # arrange | ||
| node = Node(name="node1", value_dict={"filename": "mocked_file_path"}) |
There was a problem hiding this comment.
nit: it might worth to add two more unittest to test with more than one node and no node. You should aggregate all into a single test_call through parameterized unit testing. https://medium.com/@samarthgvasist/parameterized-unit-testing-in-python-9be82fa7e17f
