Add more tests courtesy of Claude by tlmquintino · Pull Request #432 · ecmwf/eccodes

tlmquintino · 2026-03-11T08:25:07Z

Description

Claude review for missing tests. Testing for Claude capability to detect edge cases and generate non-trivial tests.

Please review attentively :-)

Contributor Declaration

By opening this pull request, I affirm the following:

All authors agree to the Contributor License Agreement.
The code follows the project's coding standards.
I have performed self-review and added comments where needed.
I have added or updated tests to verify that my changes are effective and functional.
I have run all existing tests and confirmed they pass.

Copilot

Pull request overview

This PR expands the repository’s test coverage by adding several new C++ unit-test executables and new shell-script regression tests, then wiring them into tests/CMakeLists.txt so they run under CTest.

Changes:

Added new C++ unit test executables covering bits, float conversions, value API, expressions, internal data structures, geo/nearest, and I/O.
Added a wrapper script (test_unit_extended.sh) to run the new unit test executables from CTest.
Added new shell-script tests for corrupted message handling and GRIB packing round-trips, and registered them with CTest.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 12 comments.

Show a summary per file

File	Description
tests/test_bits.cc	Adds bit-manipulation unit tests
tests/test_float_conversions.cc	Adds IBM/IEEE float conversion unit tests
tests/test_value_api.cc	Adds core get/set API unit tests
tests/test_expression.cc	Adds expression subsystem unit tests
tests/test_data_structures.cc	Adds internal array/scaling unit tests
tests/test_geo.cc	Adds geoiterator and nearest-neighbor unit tests
tests/test_io.cc	Adds I/O and corrupted/truncated input unit tests
tests/test_unit_extended.sh	Adds a CTest-invoked wrapper to run the new unit executables
tests/grib_corrupted_messages.sh	Adds a script test for corrupted/truncated GRIB/BUFR tool handling
tests/grib_packing_roundtrip.sh	Adds a script test for packing/bit-depth round-trips
tests/CMakeLists.txt	Builds the new unit executables and registers the new scripts in CTest

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-11T08:31:34Z

tests/test_bits.cc

+
+    /* Test various bit widths from 1 to 32 */
+    for (int nbits = 1; nbits <= 32; nbits++) {
+        unsigned long maxVal = (1UL << nbits) - 1;


maxVal is computed with (1UL << nbits) while nbits iterates up to 32. On platforms where unsigned long is 32-bit (e.g. Windows LLP64), shifting by 32 is undefined and can break the test. Use a 64-bit literal (1ULL) and/or handle the nbits==32 case explicitly.

Suggested change

unsigned long maxVal = (1UL << nbits) - 1;

unsigned long maxVal = (unsigned long)((1ULL << nbits) - 1ULL);

Copilot · 2026-03-11T08:31:35Z

tests/test_bits.cc

+
+    /* Test bit-level signed encode/decode */
+    for (int nbits = 2; nbits <= 32; nbits++) {
+        long maxPos = (1L << (nbits - 1)) - 1;


maxPos uses (1L << (nbits - 1)) with nbits up to 32. If long is 32-bit, 1L<<31 is undefined for signed types and can produce incorrect limits (or UB) in the signed encode/decode tests. Use an unsigned/64-bit shift (e.g. 1ULL) and cast to the desired type after computing the range.

Suggested change

long maxPos = (1L << (nbits - 1)) - 1;

long maxPos = (long)((1ULL << (nbits - 1)) - 1ULL);

Copilot · 2026-03-11T08:31:35Z

tests/test_io.cc

+    /* Either returns NULL handle or an error code */
+    if (h2 != NULL) {
+        grib_handle_delete(h2);
+    }
+    /* err should indicate a problem (at minimum not a successful full read) */


test_truncated_message() currently doesn't assert any expected outcome after attempting to read the truncated file: it will PASS even if grib_handle_new_from_file succeeds with err==GRIB_SUCCESS. Add an assertion that the call fails (e.g. h2==NULL and err!=GRIB_SUCCESS), so the test actually checks behavior.

Suggested change

/* Either returns NULL handle or an error code */

if (h2 != NULL) {

grib_handle_delete(h2);

}

/* err should indicate a problem (at minimum not a successful full read) */

/* Reading a truncated file must fail */

TEST_ASSERT(h2 == NULL);

TEST_ASSERT(err != GRIB_SUCCESS);

if (h2 != NULL) {

grib_handle_delete(h2);

}

Copilot · 2026-03-11T08:31:35Z

tests/test_expression.cc

+    grib_expression* eLogAnd = new_logical_and_expression(c, NULL, NULL);
+    TEST_ASSERT(eLogAnd != NULL);
+    TEST_ASSERT(eLogAnd->class_name() != NULL);
+
+    grib_expression* eLogOr = new_logical_or_expression(c, NULL, NULL);
+    TEST_ASSERT(eLogOr != NULL);
+    TEST_ASSERT(eLogOr->class_name() != NULL);
+
+    grib_expression* eStrCmp = new_string_compare_expression(c, NULL, NULL, NULL);
+    TEST_ASSERT(eStrCmp != NULL);
+    TEST_ASSERT(eStrCmp->class_name() != NULL);
+
+    grib_expression* eUnOp = new_unop_expression(c, NULL, NULL, NULL);
+    TEST_ASSERT(eUnOp != NULL);
+    TEST_ASSERT(eUnOp->class_name() != NULL);
+
+    grib_expression* eBinOp = new_binop_expression(c, NULL, NULL, NULL, NULL);


test_expression_class_names() constructs several expression types with NULL operands (and NULL operator function pointers for new_binop_expression). These expression implementations dereference their children in destroy()/add_dependency()/print() (see e.g. expression/LogicalAnd.cc and expression/StringCompare.cc), so creating them with NULL is not a valid use of the API and makes the test fragile. Create minimal valid operand expressions (e.g. new_true_expression, new_long_expression) and pass real operator callbacks where required.

Suggested change

grib_expression* eLogAnd = new_logical_and_expression(c, NULL, NULL);

TEST_ASSERT(eLogAnd != NULL);

TEST_ASSERT(eLogAnd->class_name() != NULL);

grib_expression* eLogOr = new_logical_or_expression(c, NULL, NULL);

TEST_ASSERT(eLogOr != NULL);

TEST_ASSERT(eLogOr->class_name() != NULL);

grib_expression* eStrCmp = new_string_compare_expression(c, NULL, NULL, NULL);

TEST_ASSERT(eStrCmp != NULL);

TEST_ASSERT(eStrCmp->class_name() != NULL);

grib_expression* eUnOp = new_unop_expression(c, NULL, NULL, NULL);

TEST_ASSERT(eUnOp != NULL);

TEST_ASSERT(eUnOp->class_name() != NULL);

grib_expression* eBinOp = new_binop_expression(c, NULL, NULL, NULL, NULL);

grib_expression* eLogAnd = new_logical_and_expression(c, eTrue, eTrue);

TEST_ASSERT(eLogAnd != NULL);

TEST_ASSERT(eLogAnd->class_name() != NULL);

grib_expression* eLogOr = new_logical_or_expression(c, eTrue, eTrue);

TEST_ASSERT(eLogOr != NULL);

TEST_ASSERT(eLogOr->class_name() != NULL);

grib_expression* eStrCmp = new_string_compare_expression(c, eTrue, eTrue, NULL);

TEST_ASSERT(eStrCmp != NULL);

TEST_ASSERT(eStrCmp->class_name() != NULL);

grib_expression* eUnOp = new_unop_expression(c, NULL, eLong, NULL);

TEST_ASSERT(eUnOp != NULL);

TEST_ASSERT(eUnOp->class_name() != NULL);

grib_expression* eBinOp = new_binop_expression(c, NULL, eLong, eLong, NULL);

Copilot · 2026-03-11T08:31:36Z

tests/grib_packing_roundtrip.sh

+set Nj = 10;
+set numberOfDataPoints = 100;
+set numberOfValues = 100;
+set values = {$(python3 -c "print(','.join(['273.15']*100))")};


The test generates the constant field values via python3 -c ..., introducing a new runtime dependency on Python for this shell test. Other existing tests avoid requiring Python at runtime (e.g. the python execution is commented out in bufr_dump_decode_python.sh). Consider generating the repeated values using pure shell/awk to keep the test environment requirements minimal.

Suggested change

set values = {$(python3 -c "print(','.join(['273.15']*100))")};

set values = {$(awk 'BEGIN{for(i=1;i<=100;i++){printf "%s", "273.15"; if(i<100) printf ","}}')};

Copilot · 2026-03-11T08:31:36Z

tests/test_data_structures.cc

+    long* data = grib_iarray_get_array(a);
+    for (int i = 0; i < 100; i++) {
+        TEST_ASSERT(data[i] == i * 10);
+    }
+    free(data);
+


Same allocator mismatch as above: grib_iarray_get_array() returns memory allocated via the ecCodes context allocator, but the test frees it with free(). Switch to grib_context_free(...) to avoid UB under custom allocators/debug builds.

Copilot · 2026-03-11T08:31:36Z

tests/test_io.cc

+    TEST_ASSERT(fp != NULL);
+    int n = 0;
+    err = grib_count_in_file(NULL, fp, &n);
+    /* A corrupted file should yield 0 valid messages */


In test_corrupted_magic(), the return code from grib_count_in_file() is ignored. For corrupted input this API typically returns a non-zero error (see grib_io.cc), so the test can pass just because n remains 0. Assert the expected error code (or at least err != GRIB_SUCCESS) in addition to checking n==0.

Suggested change

/* A corrupted file should yield 0 valid messages */

/* A corrupted file should yield an error and 0 valid messages */

TEST_ASSERT(err != GRIB_SUCCESS);

Copilot · 2026-03-11T08:31:37Z

tests/grib_corrupted_messages.sh

+set +e
+${tools_dir}/grib_ls $tempCorrupt > $tempOut 2>&1
+status=$?
+set -e
+# Should fail (non-zero exit), not crash
+echo "Truncated GRIB: grib_ls exit code=$status"
+


This script captures exit codes for the corrupted/truncated cases but never asserts anything about them. As written, the test will still exit 0 even if grib_ls/bufr_ls unexpectedly succeed on corrupted input or if they crash (segfault/abort) and return a signal-based status. Add explicit checks on $status (e.g. test $status -ne 0 and/or fail on common crash codes like 134/139) so the test can actually fail when behavior regresses.

Copilot · 2026-03-11T08:31:37Z

tests/grib_corrupted_messages.sh

+. ./include.ctest.sh
+
+label="corrupted_messages"
+
+# -----------------------------------------------
+# Test that corrupted/truncated GRIB/BUFR messages
+# are handled gracefully (no crashes, proper error codes)
+# -----------------------------------------------


This test uses Unix-specific tooling (dd, /dev/urandom, etc.) but has no Windows skip guard. Many other scripts bail out early when $ECCODES_ON_WINDOWS -eq 1; consider adding the same here to avoid breaking Windows CTest runs.

Copilot · 2026-03-11T08:31:37Z

tests/grib_packing_roundtrip.sh

+# account for header line
+test "$count" -gt 0


The grib_get_data output line-count check is effectively a no-op (test "$count" -gt 0). Since this test sets Ni=36,Nj=18 (648 points) and mentions a header line, it should assert the expected number of lines (e.g. 649) to actually validate the round-trip produced the full field.

Suggested change

# account for header line

test "$count" -gt 0

# expect 1 header line + 648 data lines

test "$count" -eq 649

shahramn · 2026-03-11T14:07:24Z

Looks like the grib_packing_roundtrip is failing

shahramn · 2026-03-12T15:19:57Z

Broken test is fixed. Rerunning the CI...

codecov-commenter · 2026-03-12T16:05:15Z

Codecov Report

❌ Patch coverage is 99.63955% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.91%. Comparing base (13d9474) to head (e73d854).
⚠️ Report is 5 commits behind head on develop.

Files with missing lines	Patch %	Lines
tests/test_bufr_api.cc	98.31%	4 Missing ⚠️
tests/test_bits.cc	98.83%	2 Missing ⚠️
tests/test_io.cc	99.42%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #432      +/-   ##
===========================================
+ Coverage    88.54%   88.91%   +0.36%     
===========================================
  Files          849      862      +13     
  Lines        63097    65039    +1942     
  Branches     11235    11292      +57     
===========================================
+ Hits         55870    57829    +1959     
+ Misses        7227     7210      -17

☔ 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.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

shahramn · 2026-03-12T19:39:59Z

The coverage has now increased from 88.54% (develop branch) to 88.91% (feature/more-tests branch)

Add more tests courtesy of Claude

18295c7

tlmquintino requested review from Copilot, joobog and shahramn March 11, 2026 08:25

Copilot started reviewing on behalf of tlmquintino March 11, 2026 08:25 View session

Copilot AI reviewed Mar 11, 2026

View reviewed changes

tlmquintino added 3 commits March 11, 2026 09:16

More tests, try to complete coverage

8f6f573

More tests, try to complete coverage

8322d77

More tests, try to complete coverage

76af7c3

shahramn self-assigned this Mar 11, 2026

shahramn added the approved-for-ci Approved to run CI on ECMWF machines label Mar 11, 2026

shahramn added 2 commits March 11, 2026 21:04

Testing: Fix broken test

fe94d07

GitHub CI: Ignore paths revised

e73d854

	unsigned long maxVal = (1UL << nbits) - 1;
	unsigned long maxVal = (unsigned long)((1ULL << nbits) - 1ULL);

	long maxPos = (1L << (nbits - 1)) - 1;
	long maxPos = (long)((1ULL << (nbits - 1)) - 1ULL);

	set values = {$(python3 -c "print(','.join(['273.15']*100))")};
	set values = {$(awk 'BEGIN{for(i=1;i<=100;i++){printf "%s", "273.15"; if(i<100) printf ","}}')};

	/* A corrupted file should yield 0 valid messages */
	/* A corrupted file should yield an error and 0 valid messages */
	TEST_ASSERT(err != GRIB_SUCCESS);

Conversation

tlmquintino commented Mar 11, 2026

Description

Contributor Declaration

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

shahramn commented Mar 11, 2026

Uh oh!

shahramn commented Mar 12, 2026

Uh oh!

codecov-commenter commented Mar 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

shahramn commented Mar 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-commenter commented Mar 12, 2026 •

edited

Loading