Conversation
|
Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.
|
| assert "A" in cards_result | ||
| assert "SIMPLE" in cards_result | ||
| assert "BITPIX" in cards_result |
There was a problem hiding this comment.
I would expect a much stricter assert statement here. Something like
| assert "A" in cards_result | |
| assert "SIMPLE" in cards_result | |
| assert "BITPIX" in cards_result | |
| cards_result == ["A", "SIMPLE", "BITPIX"] |
(this is pseudo code, I don't know that a list of strings is actually expected)
| with open(filepath, "rb") as f: | ||
| header_str, cards = parse_header(f) | ||
|
|
||
| for kw in unexpected_keywords: |
There was a problem hiding this comment.
This seems odd. Wouldn't it make more sense to run the same exact assertions whatever the data file ?
| for kw in unexpected_keywords: | |
| for kw in {"HISTORY", "COMMENT", "END"}: |
|
|
||
| def test_parse_header_no_end(): | ||
| """Test that exactly one block without END card raises exception""" | ||
| with pytest.raises(Exception): |
There was a problem hiding this comment.
Exceptions should be tested, but we need as sharp a check as possible:
- the exception type should be much more accurate since we know exactly what the problem is
- we should also match the exact error message as tightly as possible (ideally with
match="^...$) to make sure the test don't silently pass when a different exception pops up first.
There was a problem hiding this comment.
Working on the feedback right now. But right now I don't know how the tests should handle the exception because currently parse_header does
if not block or len(block) < BLOCK_SIZE:
# header looks incorrect, raising exception to fall back to
# the full Header parsing
raise ExceptionI could change it to
raise ValueError("Header has an incorrect length")| with open(filepath, "rb") as f: | ||
| header_str, cards = parse_header(f) | ||
|
|
||
| assert len(header_str) % 2880 == 0 |
There was a problem hiding this comment.
this seems odd to me: why should we accept 2880, 0 and 5760 as valid responses here ?
There was a problem hiding this comment.
I did this because the blocks in the FITS are always padded to the length 2880 regardless of the cards inside. But a header can be more than 1 block, so if the output is not a multiple of 2880 then the parsing is wrong.
But 0 should not be a valid option so Ill have to update that. I'll also leave a quick comment explaining the assertion.
| @pytest.mark.parametrize( | ||
| "filename, expected_keywords", | ||
| [ | ||
| ("data/blank.fits", ["SIMPLE", "BITPIX", "NAXIS"]), | ||
| ("data/history_header.fits", ["SIMPLE", "BITPIX", "NAXIS"]), | ||
| ("data/test0.fits", ["SIMPLE", "BITPIX", "NAXIS", "EXTEND"]), |
There was a problem hiding this comment.
Quick suggestion. A nice simplification might be.
| @pytest.mark.parametrize( | |
| "filename, expected_keywords", | |
| [ | |
| ("data/blank.fits", ["SIMPLE", "BITPIX", "NAXIS"]), | |
| ("data/history_header.fits", ["SIMPLE", "BITPIX", "NAXIS"]), | |
| ("data/test0.fits", ["SIMPLE", "BITPIX", "NAXIS", "EXTEND"]), | |
| common_keywords = ["SIMPLE", "BITPIX", "NAXIS"] | |
| @pytest.mark.parametrize( | |
| "filename, expected_keywords", | |
| [ | |
| ("data/blank.fits", common_keywords), | |
| ("data/history_header.fits", common_keywords), | |
| ("data/test0.fits", common_keywords + ["EXTEND"]), |
Etc
…words more precise
|
Here's what I changed: I didn't make any changes to the exception yet and I was also thinking that the keywords test could be a little more precise if I created some ground truth parse_header output and compared to that (kind of like test_parse_header_short_keyword) Thanks for the feedback! |
4 participants
Description
This pull request is to add tests for the low-level function parse_header in the fits package.
This is apart of my Gsoc 2026 application
I added Tests for :
expected parsed keywords
making sure keywords are skipped
incomplete header blocks error
header blocks without end error
and short keywords being parsed properly with a synthetic fits file
One thing I didn't know how to handle was the pre-commit kept raising a problem with catching the blind exceptions raised in the parse_headers test. I didn't want to update the parse_headers function myself so I was just wondering how that should be handled. Thanks!