292 flaky tests #293
292 flaky tests #293
Conversation
|
Note - I only opened a PR to trigger the CI runs. |
This reverts commit 6fa6676.
…es with cftime. THey cause some assets to fail because they alter the parser, but I think this is a window into a solution.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #293 +/- ##
==========================================
+ Coverage 98.44% 98.49% +0.04%
==========================================
Files 11 11
Lines 838 996 +158
==========================================
+ Hits 825 981 +156
- Misses 13 15 +2 ☔ View full report in Codecov by Sentry. |
|
This should fix all the MOM6 issues.
TLDR; the time bounds in the mom6 model cause a lot of issues. |
…odels in the builders, this is misleading
|
Should be ready for review. There's one line that just needs mocking - we need to patch in an empty list here, but I'm struggling to work out why mocking a dataclass attribute won't work. 595 if not dvars.variable_list:
596 raise EmptyFileError("This file contains no variables")Once we get that solved, codecov should pass & everything should be working again (touch wood). |
| def parse_access_ncfile( | ||
| cls, file: str, time_dim: str = "time" | ||
| ) -> _AccessNCFileInfo: | ||
| def parse_ncfile(cls, file: str, time_dim: str = "time") -> _NCFileInfo: | ||
| """ | ||
| Get Intake-ESM datastore entry info from an ACCESS netcdf file |
There was a problem hiding this comment.
May need to alter this description
| @@ -545,6 +544,70 @@ def parser(cls, file): | |||
| except Exception: | |||
| return {INVALID_ASSET: file, TRACEBACK: traceback.format_exc()} | |||
|
|
|||
| @classmethod | |||
| def parse_ncfile(cls, file: str, time_dim: str = "time") -> _NCFileInfo: | |||
There was a problem hiding this comment.
This is still a line-for-line copy of BaseBuilder.parse_ncfile, just with the time parser object changed. Is it workable to add a time_parser kwarg to BaseBuilder.parse_ncfile, and just inherit from there?
If you want clearer separation, your other option might be to come up with a Mixin solution that defines parse_ncfile separately (which is workable, given it's a classmethod.)
There was a problem hiding this comment.
Ahh gotcha - yeah, the first solution makes a lot more sense than what I've got here.
There was a problem hiding this comment.
Turned out to be easier from a Liskov perspective to just add the time parser as a class attribute - but I think the changes should address your points.
If you're happy, I think we should be ready to merge this in.
|
Looks good to me, modulo my comments above. |
Reverting addition of MOM6 stuff seemed to fix tests (CI :X: on initial commit is due to codecov, tests all passed).
Closes #292 ... when we understand what's going on.