Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Test CCSDS header across all instrument packet definitions #184

Merged
merged 11 commits into from
Oct 11, 2023

Conversation

bourque
Copy link
Collaborator

@bourque bourque commented Oct 3, 2023

Change Summary

Overview

This PR adds a new folder for storing tests that can be run on all instruments, and a new test that tests the existence and proper formatting of the CCSDS header in all packet definition XML files.

New Dependencies

None

New Files

  • imap_processing/tests/test_decom.py
    • Tests for the decommutation process, applied to all instruments

Deleted Files

None

Updated Files

None

Testing

This test is currently failing on the idex_packet_defintiion.xml file (which currently doesn't contain a properly formatted header) and a handful of others (due to inconsistency with considering SHCOARSE as part of the header or not).

@bourque bourque self-assigned this Oct 3, 2023
@bourque bourque added the Repo: Testing Related to testing label Oct 3, 2023
@bourque
Copy link
Collaborator Author

bourque commented Oct 3, 2023

Some open questions for discussion:

  1. Is SHCOARSE to be considered part of the header or not? Should we be consistent on how this is applied across all packet definition files?
  2. Is this a sound method for testing the header (i.e. reading in the file and comparing strings)? Or should we try to actually decom each file and check if the header is in the results?
  3. Are there any other generalized tests we can add to this file?

@bourque bourque requested review from a team, sdhoyt, greglucas, tech3371, bryan-harter, laspsandoval, GFMoraga and maxinelasp and removed request for a team October 3, 2023 17:54
Copy link
Collaborator

@greglucas greglucas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks like a nice approach and is also a good way to make sure we are consistent as a team too!

Those are all really good questions that I don't know the answer to :)

imap_processing/tests/test_decom.py Outdated Show resolved Hide resolved
imap_processing/tests/test_decom.py Outdated Show resolved Hide resolved
imap_processing/tests/test_decom.py Outdated Show resolved Hide resolved
imap_processing/tests/test_decom.py Show resolved Hide resolved
imap_processing/tests/test_decom.py Outdated Show resolved Hide resolved
Copy link
Contributor

@GFMoraga GFMoraga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great! Need to change the typo on a broader scale but that can be done in another PR

imap_processing/tests/test_decom.py Show resolved Hide resolved
imap_processing/tests/test_decom.py Outdated Show resolved Hide resolved
Copy link
Contributor

@maxinelasp maxinelasp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good as a first pass! Definitely a few outstanding questions but I don't think any that would prevent a merge.

imap_processing/tests/test_decom.py Outdated Show resolved Hide resolved
@bourque bourque requested review from greglucas and GFMoraga October 5, 2023 20:54
@bourque
Copy link
Collaborator Author

bourque commented Oct 5, 2023

Ok, I believe this is good to go and the tests should pass once SWE, Ultra, and GLOWS update their XTCE files with the fixes implemented in #185

imap_processing/tests/test_decom.py Outdated Show resolved Hide resolved
imap_processing/tests/test_decom.py Outdated Show resolved Hide resolved
@maxinelasp
Copy link
Contributor

Updated GLOWS XTCE to pass these tests in #244

@bourque
Copy link
Collaborator Author

bourque commented Oct 10, 2023

pre-commit.ci autofix

@bourque
Copy link
Collaborator Author

bourque commented Oct 10, 2023

I think we still need to update the packet definitions for SWE and Ultra with the changes from #185 to get these tests to pass.

@tech3371 @laspsandoval Could you update those files for SWE and Ultra (respectively)? Or if it is straightforward, I am happy to do it as part of this PR if you could share the Excel spreadsheet.

Copy link
Collaborator

@greglucas greglucas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just an FYI, but you can always xfail Ultra for now and update it later the same way you're doing with idex I think? Or do you want to update idex as well and remove all xfails altogether? Just giving you an out to get your tests passing so you can merge this in and then let others update and remove xfails later so the onus isn't on you to do that work.

imap_processing/tests/test_decom.py Show resolved Hide resolved
@@ -358,14 +358,14 @@
</xtce:ParameterSet>
<!-- End metadata -->
<xtce:ContainerSet>
<xtce:SequenceContainer name="CCSDSPacket" abstract="true">
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@laspsandoval Do you know what abstract="true" does here? I had to remove it in order to get these tests to pass, and the other instrument's packet definitions don't have it. I am thinking that it is not important?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I now see that removing this caused test_decom_apid_88*.py to fail, so I guess it is important afterall!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ended up just making my header tests a bit more generalized to allow for this abstract parameter.

…inerSet from testing criteria to allow for parameters to be defined within them
@bourque
Copy link
Collaborator Author

bourque commented Oct 11, 2023

Just an FYI, but you can always xfail Ultra for now and update it later the same way you're doing with idex I think? Or do you want to update idex as well and remove all xfails altogether? Just giving you an out to get your tests passing so you can merge this in and then let others update and remove xfails later so the onus isn't on you to do that work.

It was an easy fix to get the Ultra packet definitions to pass, so I just did that. I think I will leave IDEX as xfail for now -- that one will require more work.

@bourque bourque merged commit e9934fd into IMAP-Science-Operations-Center:dev Oct 11, 2023
12 checks passed
@bourque bourque deleted the test-ccsds-header branch October 11, 2023 15:44
laspsandoval pushed a commit to laspsandoval/imap_processing that referenced this pull request Nov 15, 2023
…st-ccsds-header

Test CCSDS header across all instrument packet definitions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Repo: Testing Related to testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants