diff --git a/src/test/java/com/databricks/jdbc/comparator/error/Captures.java b/src/test/java/com/databricks/jdbc/comparator/error/Captures.java index a6e469a8c..8bfe78176 100644 --- a/src/test/java/com/databricks/jdbc/comparator/error/Captures.java +++ b/src/test/java/com/databricks/jdbc/comparator/error/Captures.java @@ -1,6 +1,7 @@ package com.databricks.jdbc.comparator.error; import com.databricks.jdbc.comparator.ComparisonResult; +import java.sql.ResultSet; import java.util.function.Function; /** @@ -29,6 +30,34 @@ public static CapturedOutcome capture(JdbcCall call) { } } + /** + * Runs a driver call and returns its result on success, or the thrown {@link Throwable} itself on + * failure — so the value can be handed directly to {@link + * com.databricks.jdbc.comparator.ResultSetComparator#compare}, which dispatches on runtime type + * (ResultSet vs Throwable) and, when the error gate is on, compares thrown errors deeply. + * + *

Wrap ONLY the driver call; keep provider bookkeeping outside so harness bugs still + * propagate. + */ + public static Object resultOrThrowable(JdbcCall call) { + try { + return call.call(); + } catch (Throwable t) { + return t; + } + } + + /** Closes the value if it is a ResultSet; ignores nulls, non-ResultSets, and close errors. */ + public static void closeIfResultSet(Object value) { + if (value instanceof ResultSet) { + try { + ((ResultSet) value).close(); + } catch (Exception ignored) { + // best-effort cleanup + } + } + } + /** * Compares two captured outcomes and folds the result into a {@link ComparisonResult}. Error * diffs land in the metadata/data difference lists in the exact formats {@code csvSummary()} diff --git a/src/test/java/com/databricks/jdbc/comparator/error/ErrorComparatorTest.java b/src/test/java/com/databricks/jdbc/comparator/error/ErrorComparatorTest.java index 1e2ab6064..c34a15b7b 100644 --- a/src/test/java/com/databricks/jdbc/comparator/error/ErrorComparatorTest.java +++ b/src/test/java/com/databricks/jdbc/comparator/error/ErrorComparatorTest.java @@ -7,6 +7,7 @@ import com.databricks.jdbc.comparator.ComparisonResult; import java.sql.SQLException; +import java.util.List; import org.junit.jupiter.api.Test; /** Unit tests for the pure error capture/compare logic (no JDBC connection required). */ @@ -134,13 +135,80 @@ void offModeDisablesDeepComparison() { } @Test - void unrecognizedModeDefaultsToOffInsteadOfThrowing() { + void unrecognizedModeDefaultsToShadowInsteadOfThrowing() { String prev = System.getProperty("ERROR_COMPARISON_MODE"); try { System.setProperty("ERROR_COMPARISON_MODE", "shaddow"); // typo ErrorPolicy p = ErrorPolicy.active(); - assertEquals(ErrorPolicy.Mode.OFF, p.mode()); - assertFalse(p.isDeepComparisonEnabled()); + // Fail safe: a bad value must not throw. It falls back to the default (shadow), which is + // itself CI-safe (records DIFF rows, never fails the build). + assertEquals(ErrorPolicy.Mode.SHADOW, p.mode()); + assertTrue(p.isDeepComparisonEnabled()); + } finally { + if (prev == null) { + System.clearProperty("ERROR_COMPARISON_MODE"); + } else { + System.setProperty("ERROR_COMPARISON_MODE", prev); + } + } + } + + @Test + void unsetModeDefaultsToShadow() { + String prev = System.getProperty("ERROR_COMPARISON_MODE"); + try { + System.clearProperty("ERROR_COMPARISON_MODE"); + assertEquals(ErrorPolicy.Mode.SHADOW, ErrorPolicy.active().mode()); + } finally { + if (prev != null) { + System.setProperty("ERROR_COMPARISON_MODE", prev); + } + } + } + + // ---- ErrorDiffs gate (used by DML/Volume providers) ---- + + @Test + void errorDiffsOffModeDoesLegacyClassOnly() { + withMode( + "off", + () -> { + // Same class, different SQLState/message -> OFF must NOT emit a SQLState/message diff. + CapturedOutcome l = CapturedOutcome.threw(new SQLException("a", "42P01", 1)); + CapturedOutcome r = CapturedOutcome.threw(new SQLException("b", "08000", 2)); + assertTrue(ErrorDiffs.compare(l, r, "v ").isEmpty()); + }); + } + + @Test + void errorDiffsShadowModeComparesDeeply() { + withMode( + "shadow", + () -> { + CapturedOutcome l = CapturedOutcome.threw(new SQLException("a", "42P01", 1)); + CapturedOutcome r = CapturedOutcome.threw(new SQLException("b", "08000", 2)); + List diffs = ErrorDiffs.compare(l, r, "v "); + assertTrue(diffs.stream().anyMatch(d -> d.contains("SQLState mismatch"))); + }); + } + + @Test + void errorDiffsOffModeStillFlagsDifferingClass() { + withMode( + "off", + () -> { + CapturedOutcome l = CapturedOutcome.threw(new SQLException("a", "42P01", 1)); + CapturedOutcome r = CapturedOutcome.threw(new java.sql.SQLDataException("b", "42P01", 1)); + List diffs = ErrorDiffs.compare(l, r, "v "); + assertTrue(diffs.stream().anyMatch(d -> d.contains("exception type mismatch"))); + }); + } + + private static void withMode(String mode, Runnable body) { + String prev = System.getProperty("ERROR_COMPARISON_MODE"); + try { + System.setProperty("ERROR_COMPARISON_MODE", mode); + body.run(); } finally { if (prev == null) { System.clearProperty("ERROR_COMPARISON_MODE"); diff --git a/src/test/java/com/databricks/jdbc/comparator/error/ErrorDiffs.java b/src/test/java/com/databricks/jdbc/comparator/error/ErrorDiffs.java new file mode 100644 index 000000000..9bc623263 --- /dev/null +++ b/src/test/java/com/databricks/jdbc/comparator/error/ErrorDiffs.java @@ -0,0 +1,56 @@ +package com.databricks.jdbc.comparator.error; + +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; + +/** + * Bridges suite providers that capture their own {@link CapturedOutcome}s (DML, Volume) to the + * error comparison, while honoring the {@code ERROR_COMPARISON_MODE} gate the same way {@code + * ResultSetComparator.compareErrorsDeeply} does — so {@code off} is a true kill switch everywhere. + * + *

When the gate is on, compares errors deeply (class + SQLState + code + message). When off, + * falls back to the minimal legacy check (exception class only) so no SQLState/code/message diffs + * are emitted. + */ +public final class ErrorDiffs { + + private ErrorDiffs() {} + + /** + * Returns diff strings for a pair of outcomes where at least one threw. Empty when they agree + * (per the active mode). {@code returnedLabel} prefixes how a returned value is rendered in a + * one-sided diff (e.g. {@code "update count "}). + */ + public static List compare( + CapturedOutcome left, CapturedOutcome right, String returnedLabel) { + ErrorPolicy policy = ErrorPolicy.active(); + if (!policy.isDeepComparisonEnabled()) { + return legacyClassOnly(left, right); + } + ErrorComparison c = ErrorComparator.compare(left, right, v -> returnedLabel + v); + List diffs = new ArrayList<>(c.metadataDiffs); + diffs.addAll(c.dataDiffs); + return diffs; + } + + /** + * Legacy behavior when the gate is off: both threw → flag only a differing exception class; + * one-sided → flag which side threw. No SQLState/code/message comparison. + */ + private static List legacyClassOnly(CapturedOutcome left, CapturedOutcome right) { + if (left.threw() && right.threw()) { + String l = left.error().simpleClassName(); + String r = right.error().simpleClassName(); + return l.equals(r) + ? Collections.emptyList() + : Collections.singletonList("exception type mismatch: " + l + " vs " + r); + } + if (left.threw()) { + return Collections.singletonList( + "left threw " + left.error().simpleClassName() + " but right returned"); + } + return Collections.singletonList( + "right threw " + right.error().simpleClassName() + " but left returned"); + } +} diff --git a/src/test/java/com/databricks/jdbc/comparator/error/ErrorPolicy.java b/src/test/java/com/databricks/jdbc/comparator/error/ErrorPolicy.java index 5f3a1a8f3..3856e7995 100644 --- a/src/test/java/com/databricks/jdbc/comparator/error/ErrorPolicy.java +++ b/src/test/java/com/databricks/jdbc/comparator/error/ErrorPolicy.java @@ -5,7 +5,9 @@ * *

Modes advance with the rollout: {@code off} (legacy class-only check) → {@code shadow} * (compare and report, never fail CI) → {@code authoritative} (un-baselined error diffs fail the - * build). Read the active policy via {@link #active()}. + * build). The default is {@code shadow}: deep comparison runs and records DIFF rows, but a DIFF row + * does not fail the build (only a re-thrown Throwable does), so the default is CI-safe. Read the + * active policy via {@link #active()}. * *

Message normalization and tolerance/baseline handling are intentionally omitted for now — we * compare the raw fields (class, SQLState, vendor code, message) and will add tolerance later, from @@ -21,6 +23,12 @@ public enum Mode { private static final String MODE_PROPERTY = "ERROR_COMPARISON_MODE"; + /** + * Default when the flag is unset, empty, or unrecognized. Shadow is CI-safe (records, never + * fails). Advanced to {@code authoritative} in the final rollout PR. + */ + private static final Mode DEFAULT_MODE = Mode.SHADOW; + private final Mode mode; private ErrorPolicy(Mode mode) { @@ -28,7 +36,7 @@ private ErrorPolicy(Mode mode) { } /** - * Resolves the active policy from system properties. Defaults to {@link Mode#OFF} for null, + * Resolves the active policy from system properties. Defaults to {@link #DEFAULT_MODE} for null, * empty, or unrecognized values (the latter logs a warning) so a misconfigured flag never aborts * a comparison run. */ @@ -42,7 +50,7 @@ public static ErrorPolicy of(Mode mode) { private static Mode parseMode(String raw) { if (raw == null || raw.isEmpty()) { - return Mode.OFF; + return DEFAULT_MODE; } switch (raw.trim().toLowerCase()) { case "shadow": @@ -52,13 +60,15 @@ private static Mode parseMode(String raw) { case "off": return Mode.OFF; default: - // Fail safe: a typo in the flag must not abort the comparison run. Default to OFF - // (legacy behavior) and warn, rather than throwing from every compare() call. + // Fail safe: a typo in the flag must not abort the comparison run. Fall back to the + // default mode and warn, rather than throwing from every compare() call. System.err.println( "[comparator] Unknown ERROR_COMPARISON_MODE '" + raw - + "' (expected off|shadow|authoritative); defaulting to off."); - return Mode.OFF; + + "' (expected off|shadow|authoritative); defaulting to " + + DEFAULT_MODE.name().toLowerCase() + + "."); + return DEFAULT_MODE; } } diff --git a/src/test/java/com/databricks/jdbc/comparator/suite/PreparedStatementTypesProvider.java b/src/test/java/com/databricks/jdbc/comparator/suite/PreparedStatementTypesProvider.java index 980604182..462e1b651 100644 --- a/src/test/java/com/databricks/jdbc/comparator/suite/PreparedStatementTypesProvider.java +++ b/src/test/java/com/databricks/jdbc/comparator/suite/PreparedStatementTypesProvider.java @@ -2,6 +2,7 @@ import com.databricks.jdbc.comparator.ComparisonResult; import com.databricks.jdbc.comparator.ResultSetComparator; +import com.databricks.jdbc.comparator.error.Captures; import java.math.BigDecimal; import java.sql.*; import java.util.Arrays; @@ -183,10 +184,17 @@ public ComparisonResult execute( test.setter.set(ps1); test.setter.set(ps2); - try (ResultSet rs1 = ps1.executeQuery(); - ResultSet rs2 = ps2.executeQuery()) { - assertCloudFetchExpectation(testCase, rs1, rs2); - return ResultSetComparator.compare(label, sql, testCase.getArgs(), rs1, rs2); + // Capture executeQuery per side so a one-sided/divergent error is compared, not aborted. + Object r1 = Captures.resultOrThrowable(ps1::executeQuery); + Object r2 = Captures.resultOrThrowable(ps2::executeQuery); + try { + if (r1 instanceof ResultSet && r2 instanceof ResultSet) { + assertCloudFetchExpectation(testCase, (ResultSet) r1, (ResultSet) r2); + } + return ResultSetComparator.compare(label, sql, testCase.getArgs(), r1, r2); + } finally { + Captures.closeIfResultSet(r1); + Captures.closeIfResultSet(r2); } } } diff --git a/src/test/java/com/databricks/jdbc/comparator/suite/StatementDmlProvider.java b/src/test/java/com/databricks/jdbc/comparator/suite/StatementDmlProvider.java index e1f484470..536b71a4e 100644 --- a/src/test/java/com/databricks/jdbc/comparator/suite/StatementDmlProvider.java +++ b/src/test/java/com/databricks/jdbc/comparator/suite/StatementDmlProvider.java @@ -2,8 +2,12 @@ import com.databricks.jdbc.comparator.ComparisonResult; import com.databricks.jdbc.comparator.ResultSetComparator; +import com.databricks.jdbc.comparator.error.CapturedOutcome; +import com.databricks.jdbc.comparator.error.Captures; +import com.databricks.jdbc.comparator.error.ErrorDiffs; import java.sql.Connection; import java.sql.ResultSet; +import java.sql.SQLException; import java.sql.Statement; import java.util.ArrayList; import java.util.Collections; @@ -114,7 +118,7 @@ public ComparisonResult execute( return result; } - /** Executes DML on both connections and compares update counts and exceptions. */ + /** Executes DML on both connections and compares update counts and errors. */ private void compareDml( Connection conn1, Connection conn2, @@ -122,36 +126,24 @@ private void compareDml( String sql2, List differences, String operation) { - Object result1 = executeDml(conn1, sql1); - Object result2 = executeDml(conn2, sql2); - - if (result1 instanceof Exception && result2 instanceof Exception) { - String type1 = result1.getClass().getSimpleName(); - String type2 = result2.getClass().getSimpleName(); - if (!type1.equals(type2)) { - differences.add(operation + ": exception type mismatch: " + type1 + " vs " + type2); + CapturedOutcome left = Captures.capture(() -> executeDml(conn1, sql1)); + CapturedOutcome right = Captures.capture(() -> executeDml(conn2, sql2)); + + if (left.threw() || right.threw()) { + // An error on either side. Honor the ERROR_COMPARISON_MODE gate so `off` disables deep + // comparison here just as it does on the ResultSetComparator path (the rollout kill switch). + for (String d : ErrorDiffs.compare(left, right, "update count ")) { + differences.add(operation + ": " + d); } - } else if (result1 instanceof Exception) { + } else if (!left.value().equals(right.value())) { differences.add( - operation - + ": Thrift threw " - + result1.getClass().getSimpleName() - + " but SEA succeeded"); - } else if (result2 instanceof Exception) { - differences.add( - operation + ": Thrift succeeded but SEA threw " + result2.getClass().getSimpleName()); - } else { - if (!result1.equals(result2)) { - differences.add(operation + ": update count mismatch: " + result1 + " vs " + result2); - } + operation + ": update count mismatch: " + left.value() + " vs " + right.value()); } } - private Object executeDml(Connection conn, String sql) { + private int executeDml(Connection conn, String sql) throws SQLException { try (Statement stmt = conn.createStatement()) { return stmt.executeUpdate(sql); - } catch (Exception e) { - return e; } } diff --git a/src/test/java/com/databricks/jdbc/comparator/suite/StatementOtherProvider.java b/src/test/java/com/databricks/jdbc/comparator/suite/StatementOtherProvider.java index 6f50e4551..765afc04e 100644 --- a/src/test/java/com/databricks/jdbc/comparator/suite/StatementOtherProvider.java +++ b/src/test/java/com/databricks/jdbc/comparator/suite/StatementOtherProvider.java @@ -2,6 +2,7 @@ import com.databricks.jdbc.comparator.ComparisonResult; import com.databricks.jdbc.comparator.ResultSetComparator; +import com.databricks.jdbc.comparator.error.Captures; import java.sql.Connection; import java.sql.ResultSet; import java.sql.Statement; @@ -71,16 +72,22 @@ public ComparisonResult execute( } else if (isExecuteOnly(sql)) { try (Statement s1 = conn1.createStatement(); Statement s2 = conn2.createStatement()) { - Object result1 = s1.execute(sql); - Object result2 = s2.execute(sql); + // execute() returns a Boolean; a thrown error is captured and compared, not aborted. + Object result1 = Captures.resultOrThrowable(() -> s1.execute(sql)); + Object result2 = Captures.resultOrThrowable(() -> s2.execute(sql)); return ResultSetComparator.compare(label, sql, testCase.getArgs(), result1, result2); } } else { try (Statement s1 = conn1.createStatement(); - Statement s2 = conn2.createStatement(); - ResultSet rs1 = s1.executeQuery(sql); - ResultSet rs2 = s2.executeQuery(sql)) { - return ResultSetComparator.compare(label, sql, testCase.getArgs(), rs1, rs2); + Statement s2 = conn2.createStatement()) { + Object r1 = Captures.resultOrThrowable(() -> s1.executeQuery(sql)); + Object r2 = Captures.resultOrThrowable(() -> s2.executeQuery(sql)); + try { + return ResultSetComparator.compare(label, sql, testCase.getArgs(), r1, r2); + } finally { + Captures.closeIfResultSet(r1); + Captures.closeIfResultSet(r2); + } } } } diff --git a/src/test/java/com/databricks/jdbc/comparator/suite/StatementSelectProvider.java b/src/test/java/com/databricks/jdbc/comparator/suite/StatementSelectProvider.java index f615136a0..c0ab0eb24 100644 --- a/src/test/java/com/databricks/jdbc/comparator/suite/StatementSelectProvider.java +++ b/src/test/java/com/databricks/jdbc/comparator/suite/StatementSelectProvider.java @@ -2,6 +2,7 @@ import com.databricks.jdbc.comparator.ComparisonResult; import com.databricks.jdbc.comparator.ResultSetComparator; +import com.databricks.jdbc.comparator.error.Captures; import java.sql.Connection; import java.sql.ResultSet; import java.sql.Statement; @@ -45,12 +46,22 @@ public List getTestCases() { public ComparisonResult execute( Connection conn1, Connection conn2, TestCase testCase, String label) throws Exception { String query = testCase.getIdentifier(); + // Capture each side's executeQuery as result-or-throwable so a one-sided or divergent error is + // compared instead of aborting the case. ResultSetComparator.compare dispatches on type: + // both ResultSets -> cell comparison; a Throwable involved -> deep error comparison. try (Statement stmt1 = conn1.createStatement(); - Statement stmt2 = conn2.createStatement(); - ResultSet rs1 = stmt1.executeQuery(query); - ResultSet rs2 = stmt2.executeQuery(query)) { - assertCloudFetchExpectation(testCase, rs1, rs2); - return ResultSetComparator.compare(label, query, testCase.getArgs(), rs1, rs2); + Statement stmt2 = conn2.createStatement()) { + Object r1 = Captures.resultOrThrowable(() -> stmt1.executeQuery(query)); + Object r2 = Captures.resultOrThrowable(() -> stmt2.executeQuery(query)); + try { + if (r1 instanceof ResultSet && r2 instanceof ResultSet) { + assertCloudFetchExpectation(testCase, (ResultSet) r1, (ResultSet) r2); + } + return ResultSetComparator.compare(label, query, testCase.getArgs(), r1, r2); + } finally { + Captures.closeIfResultSet(r1); + Captures.closeIfResultSet(r2); + } } } } diff --git a/src/test/java/com/databricks/jdbc/comparator/suite/VolumeOperationsProvider.java b/src/test/java/com/databricks/jdbc/comparator/suite/VolumeOperationsProvider.java index 4638c6142..ada83ab89 100644 --- a/src/test/java/com/databricks/jdbc/comparator/suite/VolumeOperationsProvider.java +++ b/src/test/java/com/databricks/jdbc/comparator/suite/VolumeOperationsProvider.java @@ -1,9 +1,13 @@ package com.databricks.jdbc.comparator.suite; import com.databricks.jdbc.comparator.ComparisonResult; +import com.databricks.jdbc.comparator.error.CapturedOutcome; +import com.databricks.jdbc.comparator.error.Captures; +import com.databricks.jdbc.comparator.error.ErrorDiffs; import java.nio.file.Files; import java.nio.file.Path; import java.sql.Connection; +import java.sql.SQLException; import java.sql.Statement; import java.util.ArrayList; import java.util.Collections; @@ -106,33 +110,21 @@ private void compareSql( String sql2, List differences, String operation) { - Object result1 = executeSql(conn1, sql1); - Object result2 = executeSql(conn2, sql2); - - if (result1 instanceof Exception && result2 instanceof Exception) { - String type1 = result1.getClass().getSimpleName(); - String type2 = result2.getClass().getSimpleName(); - if (!type1.equals(type2)) { - differences.add(operation + ": exception type mismatch: " + type1 + " vs " + type2); + CapturedOutcome left = Captures.capture(() -> executeSql(conn1, sql1)); + CapturedOutcome right = Captures.capture(() -> executeSql(conn2, sql2)); + + if (left.threw() || right.threw()) { + // Honor the ERROR_COMPARISON_MODE gate: deep comparison when enabled, legacy class-only + // when off (the rollout kill switch), matching the ResultSetComparator path. + for (String d : ErrorDiffs.compare(left, right, "result ")) { + differences.add(operation + ": " + d); } - String msg1 = ((Exception) result1).getMessage(); - String msg2 = ((Exception) result2).getMessage(); - if (msg1 != null && msg2 != null && !msg1.equals(msg2)) { - differences.add( - operation + ": exception message mismatch: '" + msg1 + "' vs '" + msg2 + "'"); - } - } else if (result1 instanceof Exception) { - differences.add(operation + ": Thrift threw " + ((Exception) result1).getMessage()); - } else if (result2 instanceof Exception) { - differences.add(operation + ": SEA threw " + ((Exception) result2).getMessage()); } } - private Object executeSql(Connection conn, String sql) { + private boolean executeSql(Connection conn, String sql) throws SQLException { try (Statement stmt = conn.createStatement()) { return stmt.execute(sql); - } catch (Exception e) { - return e; } } }