From 78fcfd3cd4c0cabe339e6e7d501d033ebfb3fe12 Mon Sep 17 00:00:00 2001
From: Leonardo Francisco <lewohart@proton.me>
Date: Sun, 12 Apr 2026 22:51:49 -0400
Subject: [PATCH] =?UTF-8?q?tests:=20revise=20comments=20=E2=80=94=20remove?=
 =?UTF-8?q?=20debug-finding=20language=20and=20issue=20tracking?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Drop 'Regression: Issue #N' / 'Bug #N' numbering, 'dead while loop',
'the old code', 'This catches the bug where' and similar changelog-style
language from test comments. Keep concise descriptions of what each test
validates. Also simplify data-loader test headers and the verify
interrupt comment.
---
 libfprint/drivers/validity/validity_verify.c |   7 +-
 tests/test-validity.c                        | 142 +++++++------------
 2 files changed, 55 insertions(+), 94 deletions(-)

diff --git a/libfprint/drivers/validity/validity_verify.c b/libfprint/drivers/validity/validity_verify.c
index 0adf6506..e083645e 100644
--- a/libfprint/drivers/validity/validity_verify.c
+++ b/libfprint/drivers/validity/validity_verify.c
@@ -98,10 +98,9 @@ verify_interrupt_cb (FpiUsbTransfer *transfer,
               int_type, transfer->actual_length);
     }
 
-  /* During match wait, type 3 = match found, anything else = no match.
-   * python-validity treats any interrupt type != 3 as "finger not recognized"
-   * and skips cmd 0x60 (get_match_result), going straight to cleanup.
-   * The sensor only sends one interrupt, so waiting for another hangs. */
+  /* Match result: type 3 = match found, anything else = no match.
+   * On no-match, skip cmd 0x60 (get_match_result) and go to cleanup;
+   * the sensor only sends one interrupt per match attempt. */
   if (fpi_ssm_get_cur_state (ssm) == VERIFY_WAIT_MATCH_INT)
     {
       if (int_type == 3)
diff --git a/tests/test-validity.c b/tests/test-validity.c
index 91650c16..a8026edd 100644
--- a/tests/test-validity.c
+++ b/tests/test-validity.c
@@ -1377,7 +1377,7 @@ test_reboot_cmd_format (void)
 }
 
 /* ================================================================
- * Regression: fwext_file_clear on already-cleared struct
+ * fwext_file_clear on already-cleared struct
  *
  * Double-free guard.
  * ================================================================ */
@@ -2149,9 +2149,8 @@ build_match_payload (guint32       user_dbid,
 /* ================================================================
  * R1: parse_match_result with valid TLV data
  *
- * Regression: Issue #1 — dead while loop would never extract fields.
  * Verifies that user_dbid, subtype, and hash are correctly parsed
- * from a TLV dictionary matching python-validity's parse_dict() format.
+ * from a TLV dictionary.
  * ================================================================ */
 static void
 test_parse_match_result_valid (void)
@@ -2176,11 +2175,10 @@ test_parse_match_result_valid (void)
 }
 
 /* ================================================================
- * R1b: parse_match_result iterates ALL TLV entries
+ * R1b: parse_match_result iterates all TLV entries
  *
- * Regression: The dead while loop would break after first entry.
- * Build a dict with tag 3 (subtype) BEFORE tag 1 (user_dbid) to
- * ensure the parser doesn't stop after the first entry.
+ * Tags appear in non-sequential order (tag 3 before tag 1) to
+ * verify the parser processes every entry, not just the first.
  * ================================================================ */
 static void
 test_parse_match_result_multi_tags (void)
@@ -2223,7 +2221,7 @@ test_parse_match_result_multi_tags (void)
 /* ================================================================
  * R1c: parse_match_result with empty dict (no match)
  *
- * Regression: A no-match scenario should return ok=TRUE but matched=FALSE.
+ * A no-match scenario should return ok=TRUE but matched=FALSE.
  * ================================================================ */
 static void
 test_parse_match_result_empty (void)
@@ -2304,11 +2302,10 @@ test_parse_match_result_unknown_tags (void)
 }
 
 /* ================================================================
- * R2: validity_db_build_identity rejects NULL
+ * R2: validity_db_build_identity rejects NULL user_id
  *
- * Regression: Issue #2 — NULL user_id was passed to build_identity
- * which would then be passed to g_variant_new_string(NULL) → crash.
- * The guard should return NULL.
+ * A NULL user_id must return NULL instead of crashing in
+ * g_variant_new_string().
  * ================================================================ */
 static void
 test_build_identity_null (void)
@@ -2331,8 +2328,8 @@ test_build_identity_null (void)
 /* ================================================================
  * R2b: validity_db_build_identity with valid UUID
  *
- * Regression: Ensures UUID → identity bytes works correctly
- * (complementary to the NULL test above).
+ * UUID → identity bytes round-trip (complementary to the NULL
+ * test above).
  * ================================================================ */
 static void
 test_build_identity_valid_uuid (void)
@@ -2357,8 +2354,8 @@ test_build_identity_valid_uuid (void)
 /* ================================================================
  * R3: Gallery matching by subtype
  *
- * Regression: Issue #3 — identify always returned first gallery print
- * regardless of actual subtype. Now it should match by finger subtype.
+ * Identify must match by finger subtype, not just return the
+ * first gallery entry.
  * ================================================================ */
 static void
 test_gallery_match_by_subtype (void)
@@ -2432,10 +2429,9 @@ test_gallery_match_empty (void)
 }
 
 /* ================================================================
- * R4: enroll_user_dbid field exists separately from delete_storage_dbid
+ * R4: enroll_user_dbid and delete_storage_dbid are separate fields
  *
- * Regression: Issue #4 — delete_storage_dbid was abused for enrollment.
- * This compile-time test verifies both fields exist independently.
+ * Both fields must exist independently in the device struct.
  * ================================================================ */
 static void
 test_struct_separate_fields (void)
@@ -2458,10 +2454,7 @@ test_struct_separate_fields (void)
 /* ================================================================
  * R5: del_record command format
  *
- * Regression: Issue #5 — delete SSM never sent del_record cmd.
  * Verify cmd 0x48 produces correct format: 0x48 | dbid(2LE).
- * (This already exists in test-validity-db.c but we double-check
- * the format critical for delete functionality.)
  * ================================================================ */
 static void
 test_del_record_format (void)
@@ -2476,10 +2469,9 @@ test_del_record_format (void)
 }
 
 /* ================================================================
- * R6: match_finger command is exactly 13 bytes (single allocation)
+ * R6: match_finger command is exactly 13 bytes
  *
- * Regression: Issue #6 — build_cmd_match_finger allocated 12 bytes,
- * freed, then re-allocated 13 bytes. Now single allocation.
+ * build_cmd_match_finger must produce a single 13-byte buffer.
  * ================================================================ */
 static void
 test_match_finger_size (void)
@@ -2504,8 +2496,7 @@ test_match_finger_size (void)
 /* ================================================================
  * R7: Clear storage SSM states exist
  *
- * Regression: Issue #7 — clear_storage was a stub returning NOT_SUPPORTED.
- * Verify the CLEAR_* enum states exist (compile-time regression test).
+ * CLEAR_* enum states must be defined for clear_storage to work.
  * ================================================================ */
 static void
 test_clear_storage_states_exist (void)
@@ -2968,12 +2959,10 @@ test_unwrap_invalid (void)
 }
 
 /* ================================================================
- * Regression: Bug #1 — Flash parse requires PSK for private key
+ * Flash parse requires PSK before private key decryption
  *
- * Private key block (ID 4) is encrypted with PSK. Calling parse_flash
- * without first deriving PSK must fail (HMAC mismatch), proving the
- * ordering dependency. This catches the bug where flash_read SSM
- * parsed flash data BEFORE PSK derivation had occurred.
+ * Private key block (ID 4) is encrypted with PSK. parse_flash
+ * without prior PSK derivation must fail (HMAC mismatch).
  * ================================================================ */
 static void
 test_flash_parse_needs_psk (void)
@@ -2996,11 +2985,11 @@ test_flash_parse_needs_psk (void)
    * We use a minimal cert (just 16 bytes of dummy data) and a privkey block
    * that's encrypted with the proper PSK. */
 
-  /* Step 1: Build a cert body */
+  /* Build a cert body */
   guint8 cert_body[16];
   memset (cert_body, 0xAA, sizeof (cert_body));
 
-  /* Step 2: Build a private-key body encrypted with PSK */
+  /* Build a private-key body encrypted with PSK */
   guint8 priv_plaintext[96]; /* d(32) + pad for block alignment */
   memset (priv_plaintext, 0xBB, sizeof (priv_plaintext));
 
@@ -3085,12 +3074,10 @@ test_flash_parse_needs_psk (void)
 }
 
 /* ================================================================
- * Regression: Bug #2 — READ_FLASH command format
+ * READ_FLASH command format
  *
- * The READ_FLASH command must be exactly 13 bytes matching
- * python-validity: pack('<BBBHLL', 0x40, partition, 1, 0, addr, size).
- * Old code only sent 10 bytes, missing the access flag and reserved
- * field. This test verifies the command constant and expected layout.
+ * Must be exactly 13 bytes: pack('<BBBHLL', 0x40, partition, 1, 0,
+ * addr, size) — including the access flag and reserved field.
  * ================================================================ */
 static void
 test_flash_cmd_format (void)
@@ -3107,13 +3094,13 @@ test_flash_cmd_format (void)
   FP_WRITE_UINT32_LE (&cmd[5], 0x0000);  /* offset */
   FP_WRITE_UINT32_LE (&cmd[9], 0x1000);  /* size */
 
-  /* Verify total size is 13 (not 10 like the old bug) */
+  /* Verify total size is 13 */
   g_assert_cmpuint (sizeof (cmd), ==, 13);
 
   /* Verify byte layout matches python-validity's pack('<BBBHLL', ...) */
   g_assert_cmpint (cmd[0], ==, 0x40);  /* command */
   g_assert_cmpint (cmd[1], ==, 0x01);  /* partition */
-  g_assert_cmpint (cmd[2], ==, 0x01);  /* access flag (was missing) */
+  g_assert_cmpint (cmd[2], ==, 0x01);  /* access flag */
   g_assert_cmpint (cmd[3], ==, 0x00);  /* reserved lo */
   g_assert_cmpint (cmd[4], ==, 0x00);  /* reserved hi */
   /* offset at bytes 5-8 (LE uint32 = 0) */
@@ -3129,17 +3116,11 @@ test_flash_cmd_format (void)
 }
 
 /* ================================================================
- * Regression: Bug #3 — Flash response has 6-byte header
+ * Flash response has 6-byte header before block data
  *
- * After vcsfw_cmd_send strips the 2-byte VCSFW status, the flash
- * response still contains a 6-byte header: [size:4 LE][unknown:2].
- * Actual flash data starts at offset 6. Old code passed the raw
- * response directly to parse_flash(), corrupting the block parsing.
- * This test verifies:
- * 1) The 6-byte header is correctly structured (size field matches)
- * 2) parse_flash works on correctly unwrapped data (offset +6)
- * 3) The raw response differs from the unwrapped payload, proving
- *    that skipping the header is necessary
+ * After stripping the 2-byte VCSFW status, the flash response
+ * contains [size:4 LE][unknown:2] before the actual block data.
+ * parse_flash must receive data starting at offset 6.
  * ================================================================ */
 static void
 test_flash_response_header (void)
@@ -3176,26 +3157,18 @@ test_flash_response_header (void)
   g_clear_error (&error);
   validity_tls_free (&tls);
 
-  /* Verify the bug scenario: passing the raw response (with the 6-byte
-   * header) gives DIFFERENT data to the parser than the correctly unwrapped
-   * payload. The first 4 bytes of the raw response are the LE size field
-   * (0x04 0x00 0x00 0x00), which would be misinterpreted as block_id=0x0004
-   * (PRIVKEY block with size 0). This is a data corruption — the parser
-   * receives wrong input either way, but the key point is that the raw
-   * response and the unwrapped payload are NOT the same buffer content. */
+  /* The raw response (with 6-byte header) differs from the unwrapped
+   * payload — passing it directly to parse_flash would corrupt block
+   * parsing since the size field (0x04 0x00 ...) looks like block_id=4. */
   g_assert_cmpuint (sizeof (response), !=, payload_len);
   g_assert_true (memcmp (response, payload, payload_len) != 0);
 }
 
 /* ================================================================
- * Regression: Bug #4 — TLS handshake expects raw TLS records
+ * TLS handshake uses raw TLS records (not VCSFW-wrapped)
  *
- * parse_server_hello expects raw TLS records starting with a content
- * type byte (0x16 for Handshake). The old code used vcsfw_cmd_send
- * which strips 2 bytes of VCSFW status, corrupting the TLS record.
- * This test verifies that:
- * - A valid TLS Handshake record header is accepted
- * - Data prefixed with a 2-byte VCSFW status is rejected
+ * parse_server_hello expects data starting with content type 0x16.
+ * A 2-byte VCSFW status prefix would corrupt the TLS record.
  * ================================================================ */
 static void
 test_server_hello_rejects_vcsfw_prefix (void)
@@ -3279,12 +3252,10 @@ test_server_hello_rejects_vcsfw_prefix (void)
 }
 
 /* ================================================================
- * Regression: Bug #5 — Client hello has 0x44 prefix (not VCSFW cmd)
+ * Client hello uses 0x44 prefix (not a VCSFW command)
  *
- * TLS handshake messages use 0x44000000 as a 4-byte prefix, NOT a
- * standard VCSFW command byte. This test verifies the prefix and that
- * the TLS record immediately follows (no VCSFW status expected in
- * response).
+ * TLS handshake messages are prefixed with 0x44000000, followed
+ * by the raw TLS record. No VCSFW status in the response.
  * ================================================================ */
 static void
 test_client_hello_tls_prefix (void)
@@ -5195,10 +5166,7 @@ test_data_load_common_valid (void)
 }
 
 /* ================================================================
- * T8.13: Enroll db_write_enable — returns NULL with no loaded data
- *
- * Verifies that the enroll/verify path fails gracefully when the
- * runtime data files haven't been loaded (e.g., package not installed).
+ * T8.13: db_write_enable returns NULL without loaded data
  * ================================================================ */
 static void
 test_data_enroll_dbe_missing (void)
@@ -5232,10 +5200,7 @@ test_data_enroll_dbe_missing (void)
 }
 
 /* ================================================================
- * T8.14: Enroll db_write_enable — returns data when store populated
- *
- * Verifies that after loading data files, the consumers can
- * access the blob through the data store accessors.
+ * T8.14: db_write_enable returns data when store is populated
  * ================================================================ */
 static void
 test_data_enroll_dbe_loaded (void)
@@ -5295,10 +5260,7 @@ test_data_enroll_dbe_loaded (void)
 }
 
 /* ================================================================
- * T8.15: load_device — corrupt init.bin fails with error
- *
- * Verifies that a corrupted data file in the device directory
- * is detected by HMAC verification and the load is rejected.
+ * T8.15: load_device rejects corrupt HMAC
  * ================================================================ */
 static void
 test_data_load_device_corrupt (void)
@@ -5526,7 +5488,7 @@ main (int argc, char *argv[])
 
   /* VERIFY tests */
 
-  /* R1: parse_match_result regression tests (Issue #1: dead while loop) */
+  /* R1: parse_match_result */
   g_test_add_func ("/validity/verify/parse_match_result_valid",
                    test_parse_match_result_valid);
   g_test_add_func ("/validity/verify/parse_match_result_multi_tags",
@@ -5540,13 +5502,13 @@ main (int argc, char *argv[])
   g_test_add_func ("/validity/verify/match_result_clear",
                    test_match_result_clear);
 
-  /* R2: identity builder NULL regression (Issue #2: NULL crash) */
+  /* R2: identity builder */
   g_test_add_func ("/validity/verify/build_identity_null",
                    test_build_identity_null);
   g_test_add_func ("/validity/verify/build_identity_valid_uuid",
                    test_build_identity_valid_uuid);
 
-  /* R3: gallery matching by subtype (Issue #3: always returned first) */
+  /* R3: gallery matching */
   g_test_add_func ("/validity/verify/gallery_match_by_subtype",
                    test_gallery_match_by_subtype);
   g_test_add_func ("/validity/verify/gallery_match_fallback",
@@ -5554,19 +5516,19 @@ main (int argc, char *argv[])
   g_test_add_func ("/validity/verify/gallery_match_empty",
                    test_gallery_match_empty);
 
-  /* R4: struct field separation (Issue #4: field abuse) */
+  /* R4: struct field separation */
   g_test_add_func ("/validity/verify/struct_separate_fields",
                    test_struct_separate_fields);
 
-  /* R5: del_record command format (Issue #5: delete SSM non-functional) */
+  /* R5: del_record command format */
   g_test_add_func ("/validity/verify/del_record_format",
                    test_del_record_format);
 
-  /* R6: match_finger single allocation (Issue #6: double alloc) */
+  /* R6: match_finger allocation */
   g_test_add_func ("/validity/verify/match_finger_size",
                    test_match_finger_size);
 
-  /* R7: clear/delete storage SSM states (Issue #7: stub) */
+  /* R7: clear/delete storage SSM states */
   g_test_add_func ("/validity/verify/clear_storage_states",
                    test_clear_storage_states_exist);
   g_test_add_func ("/validity/verify/delete_states",
@@ -5592,7 +5554,7 @@ main (int argc, char *argv[])
   g_test_add_func ("/validity/tls/client-hello", test_build_client_hello);
   g_test_add_func ("/validity/tls/unwrap/invalid", test_unwrap_invalid);
 
-  /* Regression tests for hardware-discovered bugs */
+  /* TLS regression tests */
   g_test_add_func ("/validity/tls/regression/flash-parse-needs-psk",
                    test_flash_parse_needs_psk);
   g_test_add_func ("/validity/tls/regression/flash-cmd-format",