Skip to content

Full test coverage for GRIB1 #583

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.

Sign up for GitHub

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

Jump to bottom
Merged
pp-mo merged 9 commits into from
Dec 16, 2024
Merged

Full test coverage for GRIB1 #583

pp-mo merged 9 commits into from
Dec 16, 2024

Conversation

@trexfeathers
Copy link
Contributor

@trexfeathers trexfeathers commented Dec 10, 2024 •
edited
Loading

Closes #488

We need coverage to be file-based - we can't just mock the GribWrapper since we're planning to change to GribMessage. Probably can't mock eccodes either given how it manages a file cursor.

The files we have offer almost full coverage of our GRIB1 code (GribWrapper and _grib1_load_rules.py), but I will need to generate some artificial files (maybe by on-the-fly modification?) to fill the remaining gaps:

@codecov-commenter
Copy link

codecov-commenter commented Dec 10, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.80%. Comparing base (11169cf) to head (37f550b).
Report is 29 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #583      +/-   ##
==========================================
+ Coverage   89.60%   89.80%   +0.20%     
==========================================
  Files           8        8              
  Lines        2473     2473              
  Branches      420      420              
==========================================
+ Hits         2216     2221       +5     
+ Misses        159      155       -4     
+ Partials       98       97       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@trexfeathers
Copy link
Contributor Author

trexfeathers commented Dec 13, 2024 •
edited
Loading

I have discovered some of the timeRangeIndicator values in our code are actually not loadable. We rely on a special Eccodes key called startStep - this is generated by Eccodes and not part of the GRIB spec.

iris-grib/src/iris_grib/__init__.py

Lines 302 to 307 in 11169cf

def _compute_extra_keys(self):
"""Compute our extra keys."""
global unknown_string
self.extra_keys = {}
forecastTime = self.startStep

How to calculate startStep and its other associated keys appears to rely on eccodes/definitions/grib1/localConcepts/edzw/stepType.def. Loading a file with a timeRangeIndicator value that is not included in stepType.def causes Eccodes to error if startStep is requested. The most basic demonstration can be seen when using the grib_get command:

$ grib_get my_file.grib1 -p startStep
ECCODES ERROR   :  Unknown stepType=[114] timeRangeIndicator=[114]
ECCODES ERROR   :  startStep (Function not yet implemented)

grib_get works fine when run on a file with a timeRangeIndicator value that IS included in stepType.def.

Our codebase has handling for the following: 1, 2, 3, 4, 5, 10, 51, 113, 114, 115, 116, 117, 118, 123, 124, 125. Of these, Eccodes errors for the below codes, which are precisely those not present in stepType.def:

  • 51
  • 114
  • 115
  • 116
  • 117
  • 125

I will therefore not attempt test coverage for loading these types of files. I will remove them from the original list.

@trexfeathers
Copy link
Contributor Author

trexfeathers commented Dec 13, 2024

Found some more unreachable code. GribWrapper._get_verification_date() will only be called in circumstances when the value below does not begin with "time", i.e. 0, 1, 10:

iris-grib/src/iris_grib/__init__.py

Lines 56 to 74 in 11169cf

TIME_RANGE_INDICATORS = {
0: "none",
1: "none",
3: "time mean",
4: "time sum",
5: "time _difference",
10: "none",
# TODO #567 Further exploration of following mappings
51: "time mean",
113: "time mean",
114: "time sum",
115: "time mean",
116: "time sum",
117: "time mean",
118: "time _covariance",
123: "time mean",
124: "time sum",
125: "time standard_deviation",
}

So my new tests can never exercise the code for the other timeRangeIndicator values. I will remove those points from the checklist.

@trexfeathers trexfeathers requested a review from pp-mo December 13, 2024 17:16
@trexfeathers trexfeathers marked this pull request as ready for review December 13, 2024 17:16
pp-mo
pp-mo approved these changes Dec 16, 2024
View reviewed changes
Copy link
Member

@pp-mo pp-mo left a comment

Choose a reason for hiding this comment

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

OK I'm pretty convinced that this addresses the right areas, and I've run my own coverage check to confirm it does what you say. Good enough I think !

@pp-mo pp-mo merged commit d44fdf9 into SciTools:main Dec 16, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pp-mo pp-mo pp-mo approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Write GRIB1 loading tests to modern standards

3 participants

@trexfeathers @codecov-commenter @pp-mo