Add tests for unwrap() decorator
#253
Conversation
tests/test_misc.py
Outdated
| if operation == "multiply": | ||
| return data * multiplier | ||
| return data |
There was a problem hiding this comment.
nit: This requires some mental exercise to read. Some of that seems unnecessary.
| if operation == "multiply": | |
| return data * multiplier | |
| return data | |
| assert operation == "multiply" | |
| return data * multiplier |
tests/test_misc.py
Outdated
| self.check_digest(struct.pack('III'.encode('ascii'), 1, 2, 3)) | ||
|
|
||
|
|
||
| class TestUnwrap(unittest.TestCase): |
There was a problem hiding this comment.
nit: A fair number of these tests are testing more than one thing. Would it be possible to refactor them to be parameterized?
|
I don't fully understand the linked PR so I will defer to @millerdev for review here. |
That is valid feedback! I hope this helps a little: 2b8e324 |
tests/test_misc.py
Outdated
| self.assertEqual(process_value(5, 4), 20) | ||
| assert process_value(ri, 2) == 84 | ||
| assert process_value([10], 3) == 30 | ||
| assert process_value(5, 4) == 20 |
There was a problem hiding this comment.
Why were these not parametrized?
There are a few others as well. Not deal breaker, just curious.
There was a problem hiding this comment.
Honestly, I think I just got distracted. cf3a78a
| @pytest.mark.parametrize("value,wrap_in_list", [ | ||
| ("extracted_value", False), | ||
| (42, True), | ||
| ("nested", True), |
There was a problem hiding this comment.
Did it seem too verbose to inline the full expression here? Or is there another reason why this would not work?
| @pytest.mark.parametrize("value,wrap_in_list", [ | |
| ("extracted_value", False), | |
| (42, True), | |
| ("nested", True), | |
| @pytest.mark.parametrize("input_value, value", [ | |
| (jsonpath.DatumInContext(value="extracted_value", path=None, context=None), "extracted_value"), | |
| ([jsonpath.DatumInContext(value=42, path=None, context=None)], 42), | |
| ([jsonpath.DatumInContext(value="nested", path=None, context=None)], "nested"), |
There was a problem hiding this comment.
too verbose
Yes. It felt like unnecessary duplication, and easier to read if the parameters are only what's changing.
|
|
||
| finish: | ||
| needs: test | ||
| if: ${{ always() }} |
There was a problem hiding this comment.
What does this do? Is it like if True:?
There was a problem hiding this comment.
This comes from the Coveralls instructions for configuring parallel builds. if: ${{ always() }} ensures that the "finish" job will run whether the "test" job succeeded, failed, or was cancelled. (GitHub docs)
There was a problem hiding this comment.
👍 Thanks for explaining.
Just adds tests for changes made by contributor in commit b7a93ed . No changes to functionality.