Add missing asset type, clean up tests #351
Conversation
| 'ps4b_basic_analytic_xml_nitf', 'ps4b_udm', 'udm', 'ortho_udm2', | ||
| 'video_file', 'video_frames', 'video_metadata', 'ortho_visual', | ||
| 'visual', 'visual_xml' | ||
| 'analytic', 'visual', 'visual_xml' |
There was a problem hiding this comment.
Like "visual", "analytic" was missing from the list of default asset types. That was the core problem.
| @mock.patch('planet.scripts.types.get_asset_types', | ||
| new=mock.Mock(return_value=DEFAULT_ASSET_TYPES)) | ||
| def test_asset_type(): | ||
| check = convert_asserter(AssetType()) |
There was a problem hiding this comment.
The tests for v1 of this package very often use assertion helpers like this convert_asserter. It is unnecessary and the tests are easier to read and comprehend without it. Let's not do this in future work.
| check('visual*', ['visual', 'visual_xml']) | ||
| check('*data_*', ['metadata_aux', 'metadata_txt']) | ||
| check('analytic', ['analytic']) | ||
| check('analytic,visual', ['analytic', 'visual']) |
There was a problem hiding this comment.
Each of these checks should be its own test function. When serialized an early failure can hide later failures from being seen until the tests are run again. And serialization prevents checking in random order, which allows state-dependent bugs to lurk.
| ("*data_*", ["metadata_aux", "metadata_txt"]), | ||
| ("analytic", ["analytic"]), | ||
| ("analytic,visual", ["analytic", "visual"]), | ||
| ], |
There was a problem hiding this comment.
Pytest's parametrization is how we turn that one test with N steps into N tests of 1 step.
|
|
||
| with pytest.raises(Exception) as e: | ||
| AssetType().convert('x', None, None) | ||
| assert 'invalid choice: x' in str(e.value) |
There was a problem hiding this comment.
For the same reasons, this check should be its own separate test.
| ) | ||
| def test_asset_type_convert(pattern, type_list): | ||
| """Patterns are correctly converted to a list of assert types.""" | ||
| assert sorted(AssetType()(pattern)) == sorted(type_list) |
There was a problem hiding this comment.
The code in convert_asserter is replaced by a single line. The assertion is in the body of the test, no need to check elsewhere to see what's going on.
This fixes the last failing test and improves the test that was failing. I'm going to make some comments inline that point out some defects in the original tests and suggest better patterns that we should use in future work.
cc @jreiberkyle @kevinlacaille @JuLeeAtPlanet @karrabatcheller @mkshah605 @sarasafavi