[jalpatel11]

Summary

lib/cluster, the k-means implementation behind i.cluster, currently has no dedicated test coverage. This PR adds a pytest suite that calls the C API directly through the existing ctypes bindings, continuing the test coverage work from #7871 (lib/datetime) and #7879 (lib/arraystats, lib/rowio).

The PR adds 20 tests across 3 files, along with a shared test fixture.

lib_cluster_points_ctypes_test.py

6 tests

These tests cover the point-collection API without requiring a GRASS session:

• I_cluster_begin()
• I_cluster_point(), including rejection of points with a NULL band
• The batch point-set workflow using I_cluster_begin_point_set(), I_cluster_point_part(), and I_cluster_end_point_set()
• Dropping all-zero points in the batch workflow
• I_cluster_clear()

lib_cluster_algorithm_ctypes_test.py

12 tests

Each major step of the clustering algorithm is tested independently with expected values derived by hand from the algorithm and then confirmed against the implementation.

The suite covers:

• I_cluster_means()
• I_cluster_assign()
• I_cluster_reassign(), including both the no-op and point-moving cases
• I_cluster_sum2()
• I_cluster_separation(), including the fewer-than-two-points case
• I_cluster_distinct()
• I_cluster_nclasses()
• I_cluster_merge()
• I_cluster_reclass()

The test data uses two bands deliberately. All of these functions iterate over bands, so testing only one band could allow a loop that accidentally ignores additional bands to pass.

The I_cluster_reassign() case specifically verifies this. The point (1, 9) is closer to class 0 when considering only the first band, but moves to class 1 when both bands are considered.

lib_cluster_exec_ctypes_test.py

2 tests

These tests cover I_cluster_exec() end to end using two well-separated groups.

The tests verify:

• Final class assignments
• Checkpoint callback phase sequence
• Output signatures
• Rejection when there are not enough data points

conftest.py

Most I_cluster_*() functions call G_debug(), which requires GISRC to be set even though the clustering code does not actually access a GRASS project or mapset.

Since ctypes calls read the real process environment, the fixture uses monkeypatch to provide the required session environment. This follows the same approach used by the python/grass/temporal tests and the existing grass_temporal_gui_support_test.py pattern described in AGENTS.md.

Tests that require this fixture are marked needs_solo_run. The point-collection tests do not require a session and run without the fixture.

Testing

The full suite was tested against real GRASS builds in both:

• osgeo/grass-gis:main-ubuntu
• osgeo/grass-gis:main-alpine

Both environments are covered because a platform difference in how a missing GRASS session manifests has previously masked a failure in this test approach.

All 20 tests pass in both environments, including repeated runs with pytest-xdist.

The expected values were also independently reproduced using a small C harness linked directly against the compiled libraries rather than relying only on the ctypes test path. The harness passes cleanly under ASan.

The asserted values were checked against lib/cluster rebuilt with:

• -O0
• -O2
• -O3
• -ffast-math
• -O3 -march=native

The results were bit-identical across these builds, so the tests are not relying on floating-point tie behavior.

As an additional regression check, I introduced 15 deliberate mutations into the library, including restricting band processing to band 0 and changing behavior in means, assign, reassign, sum2, separation, merge, and the NULL/all-zero point handling. The test suite caught all 15 mutations.

I then restored the library and rebuilt it from the original source without any mutations. The unmutated build passed the full test suite, confirming that the failures were caused by the intentional mutations rather than the build or test setup.

Formatting and linting were also checked with the pinned ruff version from .pre-commit-config.yaml (v0.15.17).

Not covered

The following paths are intentionally outside the scope of this PR:

• I_cluster_exec() interrupt (-2) path
• I_cluster_exec() out-of-memory (-1) path
• The phase-4 merge branch, since the test data converges before a merge is required
• I_cluster_exec_free()

Separate issue found

While writing the tests, I found an issue in I_cluster_point_part().

The function reports a NULL value as rejected, but the point can still be retained by I_cluster_end_point_set(). The latter only checks all_zero(), which reads a band slot that was never written. This can leave the point with stale or uninitialized data and cause band_sum to become inconsistent with npoints.

The batch API is not currently used by i.cluster, which uses I_cluster_point(), so this does not affect the current tool behavior.

I am keeping this separate from the test PR rather than changing library behavior here and will address it separately.

AI Disclosure

I used AI assistance (Claude) while drafting the tests and during the verification process, including the independent C harness, ASan checks, compiler-flag checks, and mutation testing.

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

[jalpatel11]

@echoix Can you review it? I used the method you suggested this time to review code using better model.

[echoix]

If you want to take a look at what your code coverage looks like, I cherry picked your change on top of my work doing code coverage on the C code, and it shows what was touched and what missed

https://app.codecov.io/gh/echoix/grass/pull/698/indirect-changes

In c_exec, (I_cluster_exec), I think you might be able to get coverage in the argument validation code paths.

Take a look at what you think you could cover easily that would still be in scope.

[jalpatel11]

@echoix Added 8 tests covering I_cluster_exec()'s argument-validation paths: the six defaulting branches (negative maxclass, non-positive/too-small min_class_size, non-positive iterations/convergence, negative separation) plus the two interrupt-return paths that were previously untested. Since the corrected values are never stored anywhere readable, each test proves the substitution happened by contrasting the invalid input's result against an explicit boundary value that produces a different, checkable outcome.

Verified with mutation testing: recompiled libgrass_cluster.so with each defaulting if disabled and confirmed the corresponding new test catches it (5 via assertion failure, one via a hard crash, since a negative nclasses flows into I_alloc_int() and corrupts memory). Also re-verified the whole suite on both glibc and musl.

[echoix]

Thanks!