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

Add integration tests for bit shift functionality #153

Draft
wants to merge 1 commit into
base: DSEGOG-325-Update-ingestion-to-support-epac_ops_data_version-1.1
Choose a base branch
from

Conversation

patrick-austin
Copy link
Contributor

@patrick-austin patrick-austin commented Jan 29, 2025

This adds "integration" tests for the bit depth changes in #151 . It will require a single hdf5 file, generated with v1.1 of the simulated data tool for the timestamp 20230606120000 to be ingested into the bucket the tests run against. This has not yet been added to the CI to avoid failing other branches, so as a result the tests on this branch will fail. I haven't attached this file as it's fairly large, but can circulate if others want to test locally or put it into the CI bucket when we're ready.

New tests:

  • Two new test cases for test_valid_get_records:

    • That bit_depth does not appear in the metadata for old data (v1.0)
    • That bit_depth does appear in the metadata for new data (v1.1)
  • Three new test cases for test_valid_get_image:

    • 12 bit original image:
      20230606120000_CM-202-CVC-CAM-1_None

    • 12 bit image with 8 bit limits applied:
      20230606120000_CM-202-CVC-CAM-1_8

    • 12 bit image with 12 bit limits applied (this should result in the same image as in both cases these values are scaled up to the "storage" bit depth, which is 16 bits, before being applied):
      20230606120000_CM-202-CVC-CAM-1_12

  • Unit test of _bit_shift_to_storage as not all lines were covered by the above test

Existing tests updated:

  • most_recent_date and recent_sample include values for the new timestamp
  • Count records incremented by 1 for total, and greater than range
  • Range converter includes the new most recent timestamp/shotnumber conversion

@moonraker595
Copy link
Contributor

moonraker595 commented Feb 5, 2025

Hi, Just picking this up as well as I thought it'd be a good idea to pull down the bit shift code and try and run some tests against it.
Just some general questions:

  • PR 151 & 153: Just wondering if these can be merged into one branch so I can get both code and tests. I can merge them myself locally if not.

  • "This has not yet been added to the CI to avoid failing other branches"
    Was I the hold up here? If so, could this be added to the CI bucket now?

  • Side note: do you want to merge the changes to main into 151 or worry about that later? Also, I'll try and grab @kevinphippsstfc and yourself after stand up tomorrow to see if we can clean out any of the older/dangling branches.

@patrick-austin
Copy link
Contributor Author

Hi, Just picking this up as well as I thought it'd be a good idea to pull down the bit shift code and try and run some tests against it. Just some general questions:

* PR 151 & 153: Just wondering if these can be merged into one branch so I can get both code and tests. I can merge them myself locally if not.

* "This has not yet been added to the CI to avoid failing other branches"
  Was I the hold up here? If so, could this be added to the CI bucket now?

* Side note: do you want to merge the changes to main into 151 or worry about that later? Also, I'll try and grab @kevinphippsstfc and yourself after stand up tomorrow to see if we can clean out any of the older/dangling branches.

Yeah sorry I'm only looking at this comment now. I know we spoke about this earlier but to reiterate:

Happy to discuss and clarify more if needed.

@moonraker595
Copy link
Contributor

It's all good. I should've realised that it was taken from the other branch. I'm happy to wait until everything is approved for merging/adding the hdf file to the bucket. If this happens again, I think we should check the tests and maybe refactor anything that checks for an exact number of test files.

I've spun up the API, ingested the hdf file and am just running the nox stages now 👍

Copy link
Contributor

@moonraker595 moonraker595 left a comment

Choose a reason for hiding this comment

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

Tests look good. The only thing i can't see is if the code coverage has changed. I'm happy to approve so we can get the CI working with the new test data, and then create a separate PR for any code coverage concerns if needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants