From b16d1f44eed7f4c59a3af0added7dce0d9f40028 Mon Sep 17 00:00:00 2001 From: Rene Cannao Date: Sun, 22 Mar 2026 18:45:53 +0000 Subject: [PATCH] Fix review issues in FFTO E2E tests (#5516) Two critical issues identified during code review: 1. test_ffto_mysql_transactions-t.cpp: TAP plan count mismatch. kPlannedTests was declared as 28 but the actual assertion count is 34 (1 connect + 9 scenario1 + 12 scenario2 + 3 scenario3 + 9 scenario4). This would cause TAP to report "planned 28 but ran 34" and fail unconditionally. Fixed by correcting the count to 34 and cleaning up the misleading breakdown comment. 2. test_ffto_mysql_concurrent-t.cpp: undefined behavior on pthread_create failure. If pthread_create failed for thread i, threads[i] was never initialized, but pthread_join(threads[i]) was still called unconditionally. Fixed by tracking which threads were successfully created via a bool array and only joining those. Co-Authored-By: Claude Opus 4.6 (1M context) --- .../tests/test_ffto_mysql_concurrent-t.cpp | 7 ++++- .../tests/test_ffto_mysql_transactions-t.cpp | 30 +++++-------------- 2 files changed, 13 insertions(+), 24 deletions(-) diff --git a/test/tap/tests/test_ffto_mysql_concurrent-t.cpp b/test/tap/tests/test_ffto_mysql_concurrent-t.cpp index 2b0c14ab8..ba3f556c3 100644 --- a/test/tap/tests/test_ffto_mysql_concurrent-t.cpp +++ b/test/tap/tests/test_ffto_mysql_concurrent-t.cpp @@ -274,6 +274,7 @@ int main(int argc, char** argv) { thread_args args[NUM_THREADS]; pthread_t threads[NUM_THREADS]; + bool thread_created[NUM_THREADS] = {}; for (int i = 0; i < NUM_THREADS; i++) { args[i].thread_id = i; @@ -286,11 +287,15 @@ int main(int argc, char** argv) { diag("Failed to create thread %d", i); std::lock_guard lock(tap_mutex); ok(0, "Thread %d creation failed", i); + } else { + thread_created[i] = true; } } for (int i = 0; i < NUM_THREADS; i++) { - pthread_join(threads[i], NULL); + if (thread_created[i]) { + pthread_join(threads[i], NULL); + } } /* ── Aggregate stats verification ───────────────────────────────── */ diff --git a/test/tap/tests/test_ffto_mysql_transactions-t.cpp b/test/tap/tests/test_ffto_mysql_transactions-t.cpp index b772d6f02..7168341c4 100644 --- a/test/tap/tests/test_ffto_mysql_transactions-t.cpp +++ b/test/tap/tests/test_ffto_mysql_transactions-t.cpp @@ -35,31 +35,15 @@ /** * @brief Total number of planned TAP assertions. * - * Breakdown: - * - Setup: 1 (connect) - * - Scenario 1 (BEGIN/COMMIT): 4×3 = 12 (BEGIN, INSERT, INSERT digests, COMMIT) - * — INSERT shares one digest with count=2 - * → actually 3 digests × 3 assertions = 9 - * - Scenario 2 (ROLLBACK): 4×3 = 12 → 4 digests × 3 = 12 - * — but BEGIN reuses scenario 1 digest - * → 3 new digests × 3 + reuse = still verify 4 - * - * Simplified: we verify per-scenario after clearing stats each time. - * - Scenario 1: 3 verify_digest calls = 9 assertions - * - Scenario 2: 4 verify_digest calls = 12 assertions - * - Scenario 3: 1 verify_digest call = 3 assertions - * - Scenario 4: 3 verify_digest calls = 9 assertions + * Breakdown (stats cleared between scenarios): + * - Setup: 1 (connect ok) + * - Scenario 1: 3 verify_digest calls (BEGIN, INSERT, COMMIT) = 9 + * - Scenario 2: 4 verify_digest calls (BEGIN, INSERT, SELECT, ROLLBACK) = 12 + * - Scenario 3: 1 verify_digest call (prepared INSERT ×20) = 3 + * - Scenario 4: 3 verify_digest calls (SAVEPOINT, INSERT, RELEASE) = 9 * Total = 1 + 9 + 12 + 3 + 9 = 34 - * - * That's too many — let's trim to essential checks: - * - Scenario 1: verify BEGIN(1), INSERT(2 affected), COMMIT(1) = 9 - * - Scenario 2: verify ROLLBACK(1), SELECT(1 row_sent) = 6 - * - Scenario 3: verify prepared INSERT (20 executions, 20 affected) = 3 - * - Scenario 4: verify SAVEPOINT(1), INSERT(1), RELEASE(1) = 9 - * + 1 connect - * Total = 28 */ -static constexpr int kPlannedTests = 28; +static constexpr int kPlannedTests = 34; /** @copydoc FAIL_AND_SKIP_REMAINING */ #define FAIL_AND_SKIP_REMAINING(cleanup_label, fmt, ...) \