Unit test coverage for cf.py

[trexfeathers]

Description

Ref #6977

Still lots left to do; pushing up as an advanced preview.

Disclosure: heavy use of Copilot for this one, but I have carefully reviewed the full output and made modifications where needed.

To-do 2026-08-27

• Full unit CFGroup coverage
• Full unit CFReader coverage
• Full unit CFVariable coverage

When I refer to full unit coverage I am talking about a dedicated test module delivering said coverage - instead of relying on incidental coverage that is accidentally provided by other unit tests.

Checklist

Important

The Iris core developers are here to help! If anything below is unclear, just post a comment asking for help 😊

• Included a What's New entry
• Incorporated type hints in any changed code
• Checked if tests need updating
• Checked if benchmarks need updating
• Checked if documentation needs updating
  • Docstrings (we love code examples!)
  • User Manual pages
• Checked if dependencies need updating
• Confirmed that the GitHub 'checks' on this PR are passing ✅
  (further reading)

---

Tip

Things you can trigger on this PR:

• Add this label to trigger benchmarks: benchmark_this Request that this pull request be benchmarked to check if it introduces performance shifts
• Visit this URL - swapping 9999 for this PR's number - to re-trigger the CLA check:
  https://cla-assistant.io/check/SciTools/iris?pullRequest=9999

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.68%. Comparing base (cefe4b7) to head (9800816).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7259      +/-   ##
==========================================
+ Coverage   90.42%   90.68%   +0.25%     
==========================================
  Files          93       93              
  Lines       25816    25816              
  Branches     4796     4796              
==========================================
+ Hits        23344    23411      +67     
+ Misses       1693     1650      -43     
+ Partials      779      755      -24     

☔ View full report in Codecov by Harness.
📢 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.

[scitools-ci]

Templating

This PR includes changes that may be worth sharing via templating. For each file listed below, please either:

• Action the suggestion via a pull request editing/adding the relevant file in the SciTools/.github templates/ directory. ¹
• Raise an issue against the SciTools/.github repo for the above action if you really don't have 10mins spare right now. Include an assignee, to avoid it being forgotten.
• Dismiss the suggestion if the changes are not suitable for templating.

You will need to dismiss this review before this PR can be merged. Recommend the reviewer does this as their final action before merging, as this text will continually update as commits come in.

Template candidates

The following changed files are not currently templated, but their parent directories suggest they may be good candidates for a new template to be created:

• AGENTS.md

Footnotes

1. Include this text in the PR body to avoid any notifications about applying the template changes back to the source repo!
   @scitools-templating: please no update notification on: iris ↩

[trexfeathers]

NOTE: I have not yet had time to validate this test class (formula terms are hard!).

[trexfeathers]

Done: a9d1ccc

I understand why the tests are written this way, and confirmed they provide 100% coverage.

BUT: I'm sorry to say that this area of the code is so difficult to understand that I've been unable to form a good model of what good 'conceptual coverage' should look like. I.e. the cases we would want to cover regardless of the chosen Iris implementation. I can pursue this further at the reviewer's request, but I can't judge myself whether it is worth it.

[pp-mo]

Thanks for taking onboard the various tweaks which I proposed previously.
That obsoleted a lot of my pending review comments, which I've now removed.
So here, some remaining general points to think about.

I still need to take another look at the individual category tests + check I'm happy with what they do, so I might come back with some more small points.
Otherwise good!

[pp-mo]

UPDATE Older comment, but I think it stands

I'm not really understanding this term "catalog" : I looked it up + I think the meaning is not very common usage,and rather ambiguous.

So AFAICT a "test catalog" can definitely be a thing, but I'm not sure about a "catalog test".
E.G. below, the IdentifyByAttributeCatalog docstring is
"""Catalog tests for CF variables identified via a single attribute."""
I'm not clear whether this means that it "catalogs tests", or that it "contains catalog tests".
I'm pretty sure it doesn't do the first one, and I'm still not terribly clear what the second one means -- is there such a thing as a "catalog test" + if so what is that?
Search responses only seem to riff on the idea of a "Test Catalog", which also clearly means different things in different places.

So, I'd suggest renaming this, and the classes in it.
Just something like "cf_utils" might make more sense?
I know when I've done this in the past, I've used the term "Mixin" for classes.
E.G. iris/tests/integration/fast_load/test_fast_load.py,
Mixin_FieldTest: # A mixin providing common facilities for fast-load testing
So, various of our older ones were called that, but to be fair that might only be because they originally used multiple inheritance (since unittest tests must all be in classess inheriting unittest.TestCase).

[trexfeathers]

28cabf1

Agreed. This was a hangover from an earlier implementation where there were a selection of tests for each module to import. Changed to Mixin.

[pp-mo]

Tiny point
I find it more natural to always write "result == expected", read as "result equals expected".
It reads oddly to me the other way around.
I think this style can sometimes stem from translating unittest code, which goes "self.assertEqual(expected, result)" (though authors often missed this anyway!)
I suspect this was because the already-noisier syntax reads better when "expected" is a simple constant, e.g. assertEqual(1, my_long_named_thing)

[trexfeathers]

f2414e5

I don't think this is that small! I believe it's important for correct interpretation of PyTest output.

[pp-mo]

Both of these fixtures just return a function : they don't establish a per-test context or build a fresh test object.
So I don't think making them fixtures is really useful.
Especially since most of the tests here are going to inherit from 'identify_catalogue' anyway.
So, I would just put these methods in there, and remove conftest.py altogether.

[trexfeathers]

9800816

Nice, this was another hangover from previous implementations.

[trexfeathers]

Hello @trexfeathers, @pp-mo — a heads-up that this pull request's test suite has been carried into #7298, with thanks.

#7298 is the first of seven pull requests implementing the native Zarr I/O spec; it turns iris.fileformats.cf into a package so that iris.fileformats.netcdf is imported by cf/_reader.py alone. Moving ~1700 lines is only reviewable if something pins the current behaviour first, and that something is your suite. It arrives as its own commit, applied to today's unchanged cf.py, before anything moves — so the move can be shown to change nothing.

The files are carried unmodified, apart from three mocker.patch targets that had to follow CFReader into cf/_reader.py (patching the re-export would leave those tests passing without exercising anything), and tests/test_cf.py moving to tests/integration/. No assertions were touched.

One thing worth raising while it is in front of you, in identify_mixins.py's test_warn:

def operation(warn: bool):
    warnings.warn("emit at least 1 warning", category=iris.warnings.IrisUserWarning)
    result = self.CF_CLASS.identify(vars_all, warn=warn)
    # For multi-identity tests with string-typed vars, result may be empty
    # For single-identity tests, result may be non-empty

result is bound and never asserted on, where the per-class test it generalises (test_CFUGridConnectivityVariable.TestIdentify.test_warn) asserted {} == result. Generalising into a shared mixin looks right, but the check seems to have been dropped rather than parametrised — and since the sentinel warnings.warn above it can satisfy the enclosing pytest.warns on its own, the gate is a little weaker than it reads.

Deliberately not changed in #7298: editing it there would mean silently altering your in-flight pull request, and would cost the carrying commit the one property that makes it reviewable — that the suite arrives byte-identical. So it is yours to take or leave. If you would rather it were fixed on the way through, say so and I will do it there instead.

🤖 Generated with Claude Code