chore(bigquery-jdbc): update integration tests to handle different cancellation#13541
chore(bigquery-jdbc): update integration tests to handle different cancellation#13541logachev wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates integration tests for the BigQuery JDBC driver. In ITNightlyBigQueryTest, the sleep duration before cancellation is increased to 15 seconds to ensure the job is in the appropriate state, and a debug print statement is added. In ITStatementTest, the expected exception message for a timed-out query is updated. Feedback suggests removing the leftover System.out.println debugging statement and refactoring the try-catch block in ITStatementTest to use assertThrows to ensure the test fails if no exception is thrown.
| try { | ||
| statement.executeQuery(selectQuery); | ||
| } catch (SQLException e) { | ||
| assertTrue(true); | ||
| assertEquals("SQL execution canceled", e.getMessage()); | ||
| assertEquals("BigQueryException during runQuery\nJob execution was cancelled: Job timed out", e.getMessage()); | ||
| } |
There was a problem hiding this comment.
Using a try-catch block without an explicit fail statement means that if statement.executeQuery(selectQuery) unexpectedly succeeds without throwing an exception, the test will pass silently. Using assertThrows is a safer and more idiomatic way to assert that an exception is thrown.
SQLException e =
assertThrows(SQLException.class, () -> statement.executeQuery(selectQuery));
assertEquals("BigQueryException during runQuery\nJob execution was cancelled: Job timed out", e.getMessage());| System.out.println(e.getMessage()); | ||
| assertTrue(e.getMessage().contains("User requested cancellation")); |
There was a problem hiding this comment.
The System.out.println statement appears to be a leftover debugging aid. It should be removed to keep the test output clean.
| System.out.println(e.getMessage()); | |
| assertTrue(e.getMessage().contains("User requested cancellation")); | |
| assertTrue(e.getMessage().contains("User requested cancellation")); |
Due to 64cde7f now
jobs.queryAPI is used even if we request job creation (Previously it falls back tojobs.insert). Due to this, updating integration test with new error/timeout values.