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

Use current date to fetch current reference files during unit testing #7744

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

tapastro
Copy link
Contributor

@tapastro tapastro commented Jul 19, 2023

This PR addresses some bespoke header generation during unit testing - fixed dates were being provided, which could prevent testing against current reference files due to CRDS USEAFTER dates.

Checklist for maintainers

  • added entry in CHANGES.rst within the relevant release section
  • updated or added relevant tests
  • updated relevant documentation
  • added relevant milestone
  • added relevant label(s)
  • ran regression tests, post a link to the Jenkins job below.
    How to run regression tests on a PR
  • Make sure the JIRA ticket is resolved properly

@codecov
Copy link

codecov bot commented Jul 19, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (d1a60ee) 76.60% compared to head (144e42b) 76.60%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #7744   +/-   ##
=======================================
  Coverage   76.60%   76.60%           
=======================================
  Files         456      456           
  Lines       36950    36950           
=======================================
  Hits        28304    28304           
  Misses       8646     8646           
Flag Coverage Δ *Carryforward flag
nightly 77.43% <ø> (ø) Carriedforward from d1a60ee

*This pull request uses carry forward flags. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@nden
Copy link
Collaborator

nden commented Jul 20, 2023

I don't expect these to work. Nirspec tests use the original instrument model and compare to the results of the IDT team pipeline using the same model, to prove we got identical results. A similar test can/should be done if we have results for the new instrument model. These have been requested in JP-3249.
The situation with MRS tests is similar. Perhaps these tests are not needed any more.

@hbushouse
Copy link
Collaborator

I'm not sure how I feel about, in terms of the general philosophy. On the one hand, it's useful to have tests use fixed inputs and outputs, so that you can verify that new versions of the code are still giving the same answers as the old versions. At the same time, it would be useful to test the latest ref files to make sure they work properly and that our code works properly with them. The problem with the latter, as @nden already mentioned, is that we often don't have a good way of knowing whether the results from new ref files are correct or not. When it comes to WCS-related ref files and WCS-related code results, we often need to rely on the instrument teams to verify the new results, which historically is difficult to obtain.

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

Successfully merging this pull request may close these issues.

None yet

3 participants