Fix and refactor esa.hsa remote tests
#2689
Conversation
The `esa.hsa` remote tests attempted to compare sorted lists of actual checksums with the expected sorted lists, but the sorting was done using `list.sort()` in the `assert`-statement. `list.sort()` sorts the list in-place and its return value is `None`, so the tests were actually checking `None == None`. Furthermore, the checksums obtained in the tests are not reliably reproducible, so now the tests check the endings of filenames contained in the fetched tar-archives instead.
Rerunning remote tests that have failed because of a connection failure can be done with the `pytest-rerunfailures` plugin for `pytest`, so custom code for handling that in `esa.hsa` can be safely removed.
The parametrized tests are less verbose because there is less code duplication.
Codecov Report
@@ Coverage Diff @@
## main #2689 +/- ##
==========================================
+ Coverage 69.18% 69.40% +0.22%
==========================================
Files 304 304
Lines 22529 22445 -84
==========================================
- Hits 15587 15579 -8
+ Misses 6942 6866 -76
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
eerovaher
force-pushed
the
esa-hsa-remote-tests
branch
from
b568794 to
bc19107
Compare
| "method,kwargs,expected_filename,expected_endings", | ||
| [ | ||
| ("download_data", {}, "1342191813.tar", PACS_ENDINGS), | ||
| ("download_data", {"filename": "output_file"}, "output_file.tar", PACS_ENDINGS), | ||
| ("download_data", {"compress": "true"}, "1342191813.tgz", PACS_ENDINGS), | ||
| ( | ||
| "download_data", | ||
| { | ||
| "observation_id": "1342191188", | ||
| "instrument_name": "SPIRE", | ||
| "product_level": "LEVEL2", | ||
| }, | ||
| "1342191188.tar", | ||
| SPIRE_ENDINGS, | ||
| ), | ||
| ("get_observation", {}, "1342191813.tar", PACS_ENDINGS), | ||
| ], | ||
| ) |
There was a problem hiding this comment.
this is madness, empty lines for no good reason, yet logically same lines are having different indents just because black.
Please do use longer lines, manual edit, or anything that's not black to have consistency. And below, too, we do have line length up to 120, use that instead of having 3 separate lines
There was a problem hiding this comment.
There's fewer lines of code now.
There was a problem hiding this comment.
Thanks! If you know the right way to turn tools like black off on the config level, please open a PR to the config files, so I won't need to bikeshed again.
There was a problem hiding this comment.
I think the best option would be to set up an automatic code formatter that enforces a code style that is different from black, but I'm not too familiar with the alternatives. Perhaps YAPF can be configured to achieve the desired result.
Previously the unit tests in `esa.hsa` remote tests were implemented as methods of a test class, but removing the class saves a level of indentation.
eerovaher
force-pushed
the
esa-hsa-remote-tests
branch
from
bc19107 to
3a2724c
Compare
Currently many
esa.hsaremote tests are meant to compare two sorted lists, but the lists are sorted withlist.sort()in the assert statement:astroquery/astroquery/esa/hsa/tests/test_hsa_remote.py
Line 46 in a4716fd
list.sort()sorts the list in place, meaning its return value isNone:The tests are therefore asserting that
None == Noneregardless of the content of the lists. When fixing the tests it became apparent that the checksums are not reproducible, so I changed the tests to verify the filename endings in the tar-files instead.I also decided to refactor the remote test file. See the commit messages for details.