From a6b8afa1f2c2faba536adfe637d4e57b51db6e00 Mon Sep 17 00:00:00 2001 From: Rene Cannao Date: Sat, 11 Apr 2026 13:24:23 +0000 Subject: [PATCH] fix(test/tap): operator-precedence logic bug in set_testing-{,multi-,240-}t.cpp All three files had the same incorrectly-grouped failure check at the heart of their variable-comparison loop. The intended semantics were: if special_sqlmode is true AND verified_special_sqlmode is false, fail or if k == mysql_vars.end(), fail or if s == proxysql_vars["conn"].end(), fail or if special_sqlmode is false AND the inner disjunction holds, fail where the inner disjunction is (el.key() != "session_track_gtids" && values differ) OR (el.key() == "session_track_gtids" && check_session_track_gtids(...) fails) Because C++ '&&' binds tighter than '||', the source (special_sqlmode == false && (el.key() != "session_track_gtids" && (...)) || (el.key() == "session_track_gtids" && !check_session_track_gtids(...)) ) actually parsed as ( special_sqlmode == false && (el.key() != "session_track_gtids" && (...)) ) || (el.key() == "session_track_gtids" && !check_session_track_gtids(...)) i.e. the 'session_track_gtids' branch was promoted to a standalone failure condition that fired even when special_sqlmode == true (or in set_testing-240-t.cpp, also when parsing_optimizer_switch == true). This caused tests to flag spurious failures whenever session_track_gtids disagreed in the "we intentionally don't care about this iteration" cases. -Wparentheses flagged all three files. Fix is one extra pair of parentheses around the inner disjunction so it's truly guarded by the outer special_sqlmode / parsing_optimizer_switch condition. Compile-verified with -fsyntax-only -Wall on all three files. --- test/tap/tests/set_testing-240-t.cpp | 10 ++++++++-- test/tap/tests/set_testing-multi-t.cpp | 10 ++++++++-- test/tap/tests/set_testing-t.cpp | 10 ++++++++-- 3 files changed, 24 insertions(+), 6 deletions(-) diff --git a/test/tap/tests/set_testing-240-t.cpp b/test/tap/tests/set_testing-240-t.cpp index 7100bd4bd..0b9ddebab 100644 --- a/test/tap/tests/set_testing-240-t.cpp +++ b/test/tap/tests/set_testing-240-t.cpp @@ -409,15 +409,21 @@ void * my_conn_thread(void *arg) { if (std::find(possible_unknown_variables.begin(), possible_unknown_variables.end(), el.key()) != possible_unknown_variables.end()) { vars_counters[el.key()].count++; } + // The inner disjunction over session_track_gtids handling must + // be grouped so it only fires when special_sqlmode==false and + // parsing_optimizer_switch==false. Without the extra outer + // parentheses, operator precedence treated the + // 'el.key() == "session_track_gtids"' branch as a standalone + // failure condition that fired even in the other cases. if ( (special_sqlmode == true && verified_special_sqlmode == false) || (k == mysql_vars.end()) || (s == proxysql_vars["conn"].end()) || ( (parsing_optimizer_switch == true) && (optimizer_switch_matches == false) ) || - (special_sqlmode == false && parsing_optimizer_switch == false && + (special_sqlmode == false && parsing_optimizer_switch == false && ( (el.key() != "session_track_gtids" && (k.value() != el.value() || s.value() != el.value())) || (el.key() == "session_track_gtids" && !check_session_track_gtids(el.value(), s.value(), k.value())) - ) + )) ) { if ( k != mysql_vars.end() && s != proxysql_vars["conn"].end()) { if (k.value() == UNKNOWNVAR) { // mysql doesn't recognize the variable diff --git a/test/tap/tests/set_testing-multi-t.cpp b/test/tap/tests/set_testing-multi-t.cpp index de5228c37..027a8136b 100644 --- a/test/tap/tests/set_testing-multi-t.cpp +++ b/test/tap/tests/set_testing-multi-t.cpp @@ -274,12 +274,18 @@ void * my_conn_thread(void *arg) { } } + // The inner disjunction over session_track_gtids handling must + // be grouped so it is only considered when special_sqlmode is + // false. Without the extra parentheses, operator precedence + // made the 'el.key() == "session_track_gtids"' branch a + // standalone failure condition that fired even when + // special_sqlmode == true. if ( (special_sqlmode == true && verified_special_sqlmode == false) || - (special_sqlmode == false && + (special_sqlmode == false && ( (el.key() != "session_track_gtids" && (!values_equiv(k.value(), el.value()) || !values_equiv(s.value(), el.value()))) || (el.key() == "session_track_gtids" && !check_session_track_gtids(el.value(), s.value(), k.value())) - ) + )) ) { __sync_fetch_and_add(&g_failed, 1); testPassed = false; diff --git a/test/tap/tests/set_testing-t.cpp b/test/tap/tests/set_testing-t.cpp index 61f5ffa3a..750fa5f43 100644 --- a/test/tap/tests/set_testing-t.cpp +++ b/test/tap/tests/set_testing-t.cpp @@ -310,14 +310,20 @@ void * my_conn_thread(void *arg) { } } + // The inner disjunction over session_track_gtids handling must + // be grouped so it is only considered when special_sqlmode is + // false. Without the extra parentheses, operator precedence + // treated the 'el.key() == "session_track_gtids"' branch as a + // standalone failure condition that fired even when + // special_sqlmode == true. if ( (special_sqlmode == true && verified_special_sqlmode == false) || (k == mysql_vars.end()) || (s == proxysql_vars["conn"].end()) || - (special_sqlmode == false && + (special_sqlmode == false && ( (el.key() != "session_track_gtids" && (!values_equiv(k.value(), el.value()) || !values_equiv(s.value(), el.value()))) || (el.key() == "session_track_gtids" && !check_session_track_gtids(el.value(), s.value(), k.value())) - ) + )) ) { if (el.key() == "wsrep_sync_wait" && k == mysql_vars.end() && (s.value() == el.value())) { variables_tested++;