tests: Add ctypes coverage for lib/arraystats and lib/rowio

[jalpatel11]

Add ctypes unit tests for lib/arraystats and lib/rowio

Summary

As a follow-up to the lib/datetime test coverage work in #7871, I reviewed libraries under lib/ that have limited or no direct test coverage and already have generated grass.lib.* ctypes bindings.

This PR adds direct pytest coverage for two self-contained libraries:

• lib/arraystats
• lib/rowio

The tests call the C APIs directly through the existing ctypes bindings. Neither library requires a GRASS session, mapset, project, or real file I/O for the functionality covered here, so these are unit tests rather than integration tests.

The PR adds 28 tests across 3 test files.

lib/arraystats

19 tests across 2 files

lib/arraystats provides statistical calculations and classification algorithms used by tools such as v.class and d.vect.thematic to classify data into map-legend breaks.

The test suite covers:

• Basic statistics
• Equal-interval classification
• Standard-deviation classification
• Quantile classification
• Equiprobable classification
• Natural-breaks/discontinuity classification
• Algorithm dispatch through AS_class_apply_algorithm()
• Algorithm selection through AS_option_to_algorithm()

Basic statistics

lib_arraystats_basic_stats_ctypes_test.py covers AS_basic_stats() and AS_eqdrt().

One important implementation detail is explicitly covered: AS_basic_stats() takes the minimum and maximum from the first and last elements of the input rather than scanning the entire array. This means those values are only correct when the input is appropriately ordered, while the sum-based statistics remain correct for unsorted input.

Classification

lib_arraystats_classify_ctypes_test.py covers all five classification algorithms, along with the dispatcher and option parsing helpers.

AS_option_to_algorithm() is tested using a minimal zero-initialized Option structure, so the test does not need to invoke G_parser() or initialize a GRASS session.

Scope

Natural-breaks coverage is limited to a regression-style case for now. The algorithm is complex enough that adding a larger set of manually derived expected values would make the initial test suite unnecessarily difficult to maintain.

Paths that call G_fatal_error() are also not covered. These paths call exit() by default, which would terminate the pytest process rather than provide an exception that can be asserted in a unit test.

lib/rowio

9 tests

lib/rowio implements an in-memory LRU row cache around caller-provided getrow and putrow callbacks.

The tests use Python closures backed by an in-memory dictionary, so no actual files are required. The file descriptor passed through the C API is treated as an opaque value and passed through to the callbacks.

The tests cover:

• Cache hits avoiding repeated reads
• Deferred writes for rows already in the cache
• Immediate writes for rows not currently cached
• Rowio_flush() writing pending dirty rows
• LRU eviction selecting the least-recently-used row
• Writing a dirty row back before its cache slot is reused
• Rowio_fileno() returning the configured descriptor

The suite also documents two less obvious behaviors found during testing:

• Rowio_forget() does not flush a dirty row. If a modified row is forgotten before it is flushed, the modification is discarded.
• Forgetting the most recently accessed row leaves the internal R->cur shortcut pointing to the existing buffer. A subsequent Rowio_get() for that same row can return the stale buffer without calling getrow(). Accessing another row first clears the shortcut and restores the normal reload behavior.

These tests document the current implementation behavior; this PR does not change it.

lib/stats Finding

While reviewing additional libraries for potential coverage, I found an issue with the generated ctypes bindings for lib/stats.

The functions declared in stats.h using the stat_func function-pointer typedef are currently interpreted by ctypesgen as data symbols rather than callable functions. The generated bindings therefore use .in_dll() for these symbols.

As a result, calling functions such as c_count, c_sum, or c_ave through grass.lib.stats causes the Python interpreter to segfault.

I confirmed the root cause by loading the same symbols manually with the correct CFUNCTYPE signature. With the correct function signature, the functions execute normally and return the expected results.

This is out of scope for this PR since lib/stats is not part of the test coverage being added here. I will report this separately and follow up with another PR for the binding fix and corresponding test coverage. I wanted to flag it here because the issue affects actual use of grass.lib.stats, not just its testability.

Testing

The tests were verified against a real GRASS build using the osgeo/grass-gis:main-alpine Docker image.

This verifies that the tests exercise the compiled C implementations through the generated ctypes bindings rather than a mocked or standalone implementation.

Results:

• 28/28 tests passing
• Verified against the compiled GRASS libraries
• No GRASS session, mapset, or project required
• Formatting and linting verified with the pinned ruff version from .pre-commit-config.yaml (v0.15.17)

AI Disclosure

I used AI assistance (Claude) while drafting and iterating on the tests, including helping verify test cases against a real compiled lib/datetime build.

I reviewed the generated tests and validation results myself and made the final decisions about test coverage, expected behavior, and the scope of the changes.

[jalpatel11]

@ninsbl @echoix Can you review this PR?

[echoix]

If you want to know what's missed, you can try pointing your AI to that work I was refreshing after seeing your previous PR: echoix#674

It is correctly supporting the code coverage in C/C++ based library code, even when called from python. So, you'll be able to see if something is missing.

[jalpatel11]

I couldn’t run the coverage setup since it hasn’t been merged yet, so I went through the source manually and compared the 28 tests against the relevant branches. I found three gaps and pushed a fix.

AS_class_apply_algorithm() is now tested through all five dispatcher branches, AS_eqdrt() now covers the i1 == 0 case, and Rowio_get() now covers the failure path when getrow() cannot read a row.

This brings the suite to 34 tests, all verified against a real GRASS build using osgeo/grass-gis:main-alpine. Once the coverage PR lands, I’ll run it against this as well to catch anything else.

[echoix]

I ran an AI review over this PR (Opus 5 though) and had it verify the asserted values by compiling lib/arraystats/{basic,class}.c and lib/rowio/*.c standalone against stubbed libgis. Good news first: every expected value in the three test files is correct, and the ctypes plumbing is sound.

Two things are probably worth acting on:

• A real stack-buffer-overflow read in AS_class_frequencies, confirmed under ASan (class.c:450), reachable from the library's own output — the new frequencies test just misses it.
• One test asserts a guarantee rowio does not actually provide, so it passes for the wrong reason and the branch it claims to cover never runs.

Details, plus smaller coverage gaps:

Full findings

Blocking

1. lib/rowio/tests/lib_rowio_ctypes_test.py:170 — test_get_returns_none_when_getrow_fails asserts a guarantee the library does not provide.

The test passes only because no successful get precedes it. Rowio_get's cleanup guard is

if (cur == R->cur)
    R->cur = -1;

which compares a slot index (cur) against a row number (R->cur), so it usually doesn't fire. Confirmed in a harness: with nrows=1, get(3) followed by a failing get(7) leaves R->cur == 3, and a later get(3) returns the clobbered buffer ("hhhh" instead of "dddd") without calling getrow at all. So the docstring's claim that the cache is "not left in a half-cached state" is false, and the branch the commit message says this test covers never executes. Either rewrite the test to document the actual (buggy) behaviour, or fix the guard in rowio.c and extend the test to do a successful get first.

2. lib/arraystats/tests/lib_arraystats_classify_ctypes_test.py:79 — the frequencies test can't reach the out-of-bounds read.

It passes only interior breaks, so the unbounded while (data[i] <= classbreaks[j]) loop in AS_class_frequencies is never exercised. It is reachable from the library's own output: AS_class_quant({1,2,3,4}, 4, 3, ...) yields breaks {2,3,4}, and feeding those back in gives a stack-buffer-overflow read at class.c:450 under ASan.

3. lib/arraystats/tests/lib_arraystats_classify_ctypes_test.py:88 — test_class_discont_on_a_uniform_range is toolchain-dependent.

Perfectly uniform data is the one input where the residual d is mathematically 0, so the d <= dmax comparison is decided by ~1e-17 of rounding noise. Measured: gcc -O0/-O2/-O3 -march=native and clang all give 0.125 / [2.5, 5.5]; gcc with -ffast-math gives 0.316 / [2.5, 10.0]. The same pin exists in the CLASS_DISCONT row of the dispatcher test. Non-uniform data would keep the branch outcome from being decided by the compiler.

Coverage gaps

• classify_ctypes_test.py:88 — no degenerate-input discont case. All-equal values, or count == 1, give rangemax == 0 → 0/0 → NaN breaks plus the sentinel chi2 = 1000, and AS_class_apply_algorithm's finfo == 0 check does not catch it (harness-verified).
• classify_ctypes_test.py:69 — AS_class_equiprob's class-reduction branch (the warning, the partial zeroing, the *nbreaks rewrite) is untested. [1]*9 + [100] with nbreaks=9 triggers it.
• classify_ctypes_test.py:131 — byref(c_int(nbreaks)) creates a throwaway, so the CLASS_EQUIPROB case cannot observe the *nbreaks write-back, which is the entire reason that parameter is a pointer. Binding the c_int to a local would let it be asserted.
• basic_stats_ctypes_test.py:26 — only 4 of the 10 GASTATS fields are asserted, and all inputs are positive, so sum/sumabs and mean/meanabs are indistinguishable; swapping them in basic.c would keep the suite green. A negative-value case plus the remaining fields would fix both.
• basic_stats_ctypes_test.py:58 — c/rc is declared but never asserted, so a regression that spuriously sets *c for a non-vertical line passes. AS_class_discont branches on exactly that value.
• lib_rowio_ctypes_test.py:56 — missing the negative-row guards (Rowio_get(-1) → NULL, Rowio_put(..., -1) → 0) and the documented putrow == NULL configuration, which pageout() would otherwise call unconditionally on a dirty row.

Note (not blocking)

lib_rowio_ctypes_test.py:93 — these LRU assertions are the first code to depend on rcb[].age, which Rowio_setup never initializes (it G_mallocs and sets only .row), and my_select bumps the age of unused slots on every get. It's benign at nrows=2 because the free-slot break wins first, but any later case with nrows >= 3 that evicts while slots are still unused would be nondeterministic. Worth a comment so the next person doesn't extend it.

Verified as correct

min/max/mean/stdev, interval, quant, stdev, equiprob, the frequency counts, and the discont chi2/breaks all reproduce. The CFUNCTYPE signatures match ctypesgen's generated argtypes, both libraries are in the interface-generator module list, the filenames match the */tests/*_test.py discovery pattern in pyproject.toml, and the style follows the lib/datetime/tests precedent.

[echoix]

So, the tests here are fine. There’s some issues to file in different PRs. (Not here). There’s still some coverage gaps. And testing for a wrong guarantee. Feel free to challenge that with an adversarial model, and your head.

[nilason]

[…] and your head.

❗

[nilason]

In general, I support this kind of low level unit tests on the libraries, and via Python bridge is an added plus.

[echoix]

And if I finish to choose the cleanest implementation of code coverage (for c based code), properly done its about 10-18% impact, and PRs like that really show what is missed or improved by these tests. I’m still learning how to use Claude, and I really like it, as it can finalize work that I started, but get stuck on a small problem and it stalled. So it unblocked me this week.

[jalpatel11]

@echoix I fixed the blocking rowio issue. The existing test_get_returns_none_when_getrow_fails was too broad, so I narrowed it to the case it actually covers: a row that has never been successfully cached. I also added test_get_after_a_failure_can_return_a_stale_buffer, which reproduces the exact case you reported (nrows=1, get(3), followed by a failing get(7)). The test documents the stale-buffer behavior as the current implementation rather than treating it as expected behavior.

The underlying issue is in Rowio_get cleanup, where a slot index is being compared against a row number. Fixing that belongs in lib/rowio, so I’m keeping it separate from this PR.

For AS_class_frequencies, I confirmed the overflow independently. Feeding the output of AS_class_quant for [1,2,3,4] back into the function produces frequency counts that sum to 6 instead of 4, which confirms memory corruption even without ASan. I’m not adding a test that intentionally triggers undefined behavior; I’ll report this separately and handle the fix in lib/arraystats.

I’m still working through the remaining non-blocking coverage gaps you mentioned, including the equiprob write-back, GASTATS fields, and negative-row guards. I’ll follow up once those are covered.

[jalpatel11]

@echoix, I reviewed the rest and closed the remaining gaps.

The discont toolchain issue is now fixed in both places. I switched the test data to clustered values so the breaks are stable and stopped pinning chi2, since it changes significantly under -ffast-math. I verified this by compiling basic.c and class.c both with and without -ffast-math, and the assertions now pass in both builds.

I also added coverage for the equiprob class-reduction branch and *nbreaks write-back, all 10 GASTATS fields with mixed-sign data, the rowio negative-row guards and putrow == NULL mode, and the c output from AS_eqdrt(). I also cleaned up the uninitialized age issue in the LRU test and added a degenerate discont case.

That brings the suite to 41 tests. The AS_class_frequencies overflow and the discont NaN behavior are library issues rather than test issues, so I’ve left both for separate PRs as discussed.