SQL: test coverage for JdbcResultSet #32813

astefan · 2018-08-13T15:27:31Z

Before this PR, the JdbcResultSet class had almost no tests. This PR tries to add a more complete set of tests for this class. Some issues were fixed in the process as well.

Fix for #32078

elasticmachine · 2018-08-13T15:27:33Z

Pinging @elastic/es-search-aggs

colings86

@astefan I did a general review of the code. I think we should get either @nik9000 or @imotov to have a look too since they are more aware of the SQL specifics

colings86 · 2018-08-14T09:06:20Z

...sql/jdbc/src/test/java/org/elasticsearch/xpack/sql/jdbc/jdbc/JdbcPreparedStatementTests.java

@@ -550,7 +550,8 @@ public void testThrownExceptionsWhenSettingByteArrayValues() throws SQLException
    }

    private long randomMillisSinceEpoch() {
-        return randomLongBetween(0, System.currentTimeMillis());
+        // random between Jan 1st, 1970 and Jan 1st, 2050


Mostly just curious: Why this end bound? Also, should we also be testing epochMillis that are before 1970-01-01 as well since they are perfectly valid?

I saw this one before in ResultSetBaseTestCase. Maybe it's time to move it to ESTestCase along with other random*(....) methods. I agree with @colings86 - it would be nice to get negative timestamps from time to time for testing. But I am +1 on constant upper bound since it will make the failure reproducible in (possibly very rare) case it somehow fails because of a particular timestamp.

@colings86 @imotov since the suggestion is to move this to ESTestCase base class (and be part of the test infra in ES), any good ideas for the interval start date?

I'm not sure why randomLong() is not good enough here? All long values are valid timestamps since epoch so should be not just use that?

👍 That sounds like a great idea. As long as we don't pass that to H2 we should be fine.

colings86 · 2018-08-14T09:08:17Z

...sql/no-security/src/test/java/org/elasticsearch/xpack/qa/sql/nosecurity/JdbcResultSetIT.java

+import org.elasticsearch.xpack.qa.sql.jdbc.ResultSetTestCase;
+
+public class JdbcResultSetIT extends ResultSetTestCase {
+}


Could you add a JavaDoc here that just describes why nothing needs to be added to this class, at first glance it looks quite strange to extends a test class and not add any test or override any methods. I'm sure there is a reason but its not obvious on first looking why doing this is right

Yeah, that confuses people. The reasons for this is that we want to run the same tests in two or three very different integration clusters. We should probably update all tests like this with comments explaining it.

colings86 · 2018-08-14T09:19:54Z

x-pack/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/jdbc/ResultSetTestCase.java

+import static java.util.Calendar.SECOND;
+import static java.util.Calendar.YEAR;
+
+public class ResultSetTestCase extends ResultSetBaseTestCase {


There is quite a lot of repetition in this class. I wonder if there is a way to create some helper methods that take functions/lambdas to reduce it? There is a balance between reducing repetition and making the test easy to follow but I wonder if there are even some smaller things we can do to help here?

imotov

In addition to what @colings86 said, I left a minor comment about consistency in messages. Otherwise, LGTM.

imotov · 2018-08-16T13:48:51Z

...sql/jdbc/src/test/java/org/elasticsearch/xpack/sql/jdbc/jdbc/JdbcPreparedStatementTests.java

@@ -92,7 +92,7 @@ public void testThrownExceptionsWhenSettingStringValues() throws SQLException {
        JdbcPreparedStatement jps = createJdbcPreparedStatement();

        SQLException sqle = expectThrows(SQLException.class, () -> jps.setObject(1, "foo bar", Types.INTEGER));
-        assertEquals("Conversion from type [VARCHAR] to [Integer] not supported", sqle.getMessage());
+        assertEquals("Unable to convert value [foo bar] to an Integer", sqle.getMessage());


We should probably be consistent in the messages. Half of the messages say "Unable to convert" another half "Conversion from type " I understand that in one case we bailout earlier by just looking at type, but in other case we are actually trying to parse the string. But I wonder if it would makes sense to have the same type of message and specify value and type in both cases.

imotov · 2018-08-16T14:02:39Z

...sql/jdbc/src/test/java/org/elasticsearch/xpack/sql/jdbc/jdbc/JdbcPreparedStatementTests.java

@@ -550,7 +550,8 @@ public void testThrownExceptionsWhenSettingByteArrayValues() throws SQLException
    }

    private long randomMillisSinceEpoch() {
-        return randomLongBetween(0, System.currentTimeMillis());
+        // random between Jan 1st, 1970 and Jan 1st, 2050


I saw this one before in ResultSetBaseTestCase. Maybe it's time to move it to ESTestCase along with other random*(....) methods. I agree with @colings86 - it would be nice to get negative timestamps from time to time for testing. But I am +1 on constant upper bound since it will make the failure reproducible in (possibly very rare) case it somehow fails because of a particular timestamp.

imotov · 2018-08-16T14:04:23Z

...sql/no-security/src/test/java/org/elasticsearch/xpack/qa/sql/nosecurity/JdbcResultSetIT.java

+import org.elasticsearch.xpack.qa.sql.jdbc.ResultSetTestCase;
+
+public class JdbcResultSetIT extends ResultSetTestCase {
+}


Yeah, that confuses people. The reasons for this is that we want to run the same tests in two or three very different integration clusters. We should probably update all tests like this with comments explaining it.

…o 32078_fix

astefan · 2018-08-22T22:28:08Z

@imotov I'm not too convinced about the common error message that should include both the value and the type of the field. The messages are quite different: trying to parse a string and failing means that we support converting from String to X, but we fail at parsing that string as a number (most common case), while in the other case, as you said, we know that a conversion from a type X to type Y is not possible according to the spec. Do you have a proposed message for the common error to be used in both cases?
@colings86 I've simplified the code and re-used some of it as much as possible. If there is anything that I can improve further, please let me know.
@nik9000 if you have a bit of time to have a look at this PR, I would appreciate it.

nik9000

I left a few minor things. Sorry it took so long for me to get to it. I'm happy with all of the extra tests and clean up though.

nik9000 · 2018-08-23T18:31:44Z

x-pack/plugin/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/jdbc/TypeConverter.java

@@ -56,9 +56,10 @@ private TypeConverter() {

    }

-    private static final long DAY_IN_MILLIS = 60 * 60 * 24;
+    private static final long DAY_IN_MILLIS = 60 * 60 * 24 * 1000;


nik9000 · 2018-08-23T18:33:31Z

x-pack/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/jdbc/ResultSetBaseTestCase.java

+        client().performRequest(request);
+    }
+
+    protected void updateMapping(String index, CheckedConsumer<XContentBuilder, IOException> body) throws Exception {


I'd make all of the protected methods final if we don't intend to override them. Just a little bit of extra signalling for the reader.

nik9000 · 2018-08-23T18:35:39Z

x-pack/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/jdbc/ResultSetBaseTestCase.java

+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+
+abstract class ResultSetBaseTestCase extends JdbcIntegrationTestCase {


Since this class only has one extension I'd fold it into the extension. Unless you have other extension that you plan to build soon.

nik9000 · 2018-08-23T18:36:57Z

...k/qa/sql/security/src/test/java/org/elasticsearch/xpack/qa/sql/security/JdbcResultSetIT.java

+ * and the Security is enabled) scenario. Runs all tests in the base class, the only changed
+ * bit is the "environment".
+ */
+public class JdbcResultSetIT extends ResultSetTestCase {


Is enough different for this to be worth testing with security?

…ails to be converted and the value itself

astefan · 2018-08-28T17:51:09Z

@costin @nik9000 I've incorporated the latest review comments.

astefan · 2018-08-29T05:14:23Z

@elasticmachine test this please!

nik9000

I left one thing that I think you should do before merging, otherwise, looks good to me.

nik9000 · 2018-08-29T16:21:02Z

x-pack/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/jdbc/ResultSetTestCase.java

+import static java.util.Calendar.MINUTE;
+import static java.util.Calendar.MONTH;
+import static java.util.Calendar.SECOND;
+import static java.util.Calendar.YEAR;

 public class ResultSetTestCase extends JdbcIntegrationTestCase {


Since we only have a single extension to this class, should we fold them together?

@nik9000 JdbcIntegrationTestCase has multiple extensions. Did you mean a different class, or I am missing something? You have a previous comment about the previous commit regarding the classes hierarchy, and I already incorporated the previous class into ResultSetTestCase.

astefan · 2018-08-31T08:04:49Z

@elasticmachine test this please!

…o 32078_fix

* Tests for JdbcResultSet * Added VARCHAR conversion for different types * Made error messages consistent: they now contain both the type that fails to be converted and the value itself

* master: Mute test watcher usage stats output [Rollup] Fix FullClusterRestart test Adjust soft-deletes version after backport into 6.5 completely drop `index.shard.check_on_startup: fix` for 7.0 (#33194) Fix AwaitsFix issue number Mute SmokeTestWatcherWithSecurityIT testsi drop `index.shard.check_on_startup: fix` (#32279) tracked at [DOCS] Moves ml folder from x-pack/docs to docs (#33248) [DOCS] Move rollup APIs to docs (#31450) [DOCS] Rename X-Pack Commands section (#33005) TEST: Disable soft-deletes in ParentChildTestCase Fixes SecurityIntegTestCase so it always adds at least one alias (#33296) Fix pom for build-tools (#33300) Lazy evaluate java9home (#33301) SQL: test coverage for JdbcResultSet (#32813) Work around to be able to generate eclipse projects (#33295) Highlight that index_phrases only works if no slop is used (#33303) Different handling for security specific errors in the CLI. Fix for #33230 (#33255) [ML] Refactor delimited file structure detection (#33233) SQL: Support multi-index format as table identifier (#33278) MINOR: Remove Dead Code from PathTrie (#33280) Enable forbiddenapis server java9 (#33245)

* 6.x: Mute test watcher usage stats output [Rollup] Fix FullClusterRestart test TEST: Disable soft-deletes in ParentChildTestCase TEST: Disable randomized soft-deletes settings Integrates soft-deletes into Elasticsearch (#33222) drop `index.shard.check_on_startup: fix` (#32279) Fix AwaitsFix issue number Mute SmokeTestWatcherWithSecurityIT testsi [DOCS] Moves ml folder from x-pack/docs to docs (#33248) TEST: mute more SmokeTestWatcherWithSecurityIT tests [DOCS] Move rollup APIs to docs (#31450) [DOCS] Rename X-Pack Commands section (#33005) Fixes SecurityIntegTestCase so it always adds at least one alias (#33296) TESTS: Fix Random Fail in MockTcpTransportTests (#33061) (#33307) MINOR: Remove Dead Code from PathTrie (#33280) (#33306) Fix pom for build-tools (#33300) Lazy evaluate java9home (#33301) SQL: test coverage for JdbcResultSet (#32813) Work around to be able to generate eclipse projects (#33295) Different handling for security specific errors in the CLI. Fix for #33230 (#33255) [ML] Refactor delimited file structure detection (#33233) SQL: Support multi-index format as table identifier (#33278) Enable forbiddenapis server java9 (#33245) [MUTE] SmokeTestWatcherWithSecurityIT flaky tests Add region ISO code to GeoIP Ingest plugin (#31669) (#33276) Don't be strict for 6.x Update serialization versions for custom IndexMetaData backport Replace IndexMetaData.Custom with Map-based custom metadata (#32749) Painless: Fix Bindings Bug (#33274) SQL: prevent duplicate generation for repeated aggs (#33252) TEST: Mute testMonitorClusterHealth Fix serialization of empty field capabilities response (#33263) Fix nested _source retrieval with includes/excludes (#33180) [DOCS] TLS file resources are reloadable (#33258) Watcher: Ensure TriggerEngine start replaces existing watches (#33157) Ignore module-info in jar hell checks (#33011) Fix docs build after #33241 [DOC] Repository GCS ADC not supported (#33238) Upgrade to latest Gradle 4.10 (#32801) Fix/30904 cluster formation part2 (#32877) Move file-based discovery to core (#33241) HLRC: add client side RefreshPolicy (#33209) [Kerberos] Add unsupported languages for tests (#33253) Watcher: Reload properly on remote shard change (#33167) Fix classpath security checks for external tests. (#33066) [Rollup] Only allow aggregating on multiples of configured interval (#32052) Added deprecation warning for rescore in scroll queries (#33070) Apply settings filter to get cluster settings API (#33247) [Rollup] Re-factor Rollup Indexer into a generic indexer for re-usability (#32743) HLRC: create base timed request class (#33216) HLRC: Use Optional in validation logic (#33104) Painless: Add Bindings (#33042)

astefan added 2 commits August 13, 2018 17:34

Tests and fixes for JdbcResultSet

deb0b03

Merge remote-tracking branch 'remotes/origin/master' into 32078_fix

34bd90f

astefan added >enhancement v7.0.0 :Analytics/SQL SQL querying v6.5.0 labels Aug 13, 2018

astefan requested review from nik9000, colings86 and imotov August 13, 2018 15:27

colings86 reviewed Aug 14, 2018

View reviewed changes

imotov reviewed Aug 16, 2018

View reviewed changes

astefan added 4 commits August 21, 2018 07:33

Replaced randomMillisSinceEpoch() with randomLong()

54834c9

Simplified code that can be re-used

7ce8fd8

Merge branch 'master' of https://github.com/elastic/elasticsearch int…

9d802a9

…o 32078_fix

Reverted small change

906ca43

nik9000 reviewed Aug 23, 2018

View reviewed changes

astefan added 3 commits August 28, 2018 20:08

Made error messages consistent: they now contain both the type that f…

f14cb8a

…ails to be converted and the value itself

Merge remote-tracking branch 'remotes/origin/master' into 32078_fix

493fc85

JdbcResultSet tests are probably not needed for with-security setup

c32a997

astefan requested a review from costin August 28, 2018 17:49

nik9000 approved these changes Aug 29, 2018

View reviewed changes

astefan added 2 commits August 31, 2018 12:24

Merge branch 'master' of https://github.com/elastic/elasticsearch int…

24ec3bf

…o 32078_fix

Merge branch 'master' of https://github.com/elastic/elasticsearch int…

6325807

…o 32078_fix

astefan merged commit 0c4b316 into elastic:master Aug 31, 2018

jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SQL: test coverage for JdbcResultSet #32813

SQL: test coverage for JdbcResultSet #32813

astefan commented Aug 13, 2018

elasticmachine commented Aug 13, 2018

colings86 left a comment

colings86 Aug 14, 2018

imotov Aug 16, 2018

astefan Aug 17, 2018

colings86 Aug 17, 2018

imotov Aug 17, 2018

colings86 Aug 14, 2018

imotov Aug 16, 2018

colings86 Aug 14, 2018

imotov left a comment

imotov Aug 16, 2018

imotov Aug 16, 2018

imotov Aug 16, 2018

astefan commented Aug 22, 2018

nik9000 left a comment

nik9000 Aug 23, 2018

nik9000 Aug 23, 2018

nik9000 Aug 23, 2018

nik9000 Aug 23, 2018

astefan commented Aug 28, 2018

astefan commented Aug 29, 2018

nik9000 left a comment

nik9000 Aug 29, 2018

astefan Aug 30, 2018

astefan commented Aug 31, 2018

SQL: test coverage for JdbcResultSet #32813

SQL: test coverage for JdbcResultSet #32813

Conversation

astefan commented Aug 13, 2018

elasticmachine commented Aug 13, 2018

colings86 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imotov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astefan commented Aug 22, 2018

nik9000 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astefan commented Aug 28, 2018

astefan commented Aug 29, 2018

nik9000 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astefan commented Aug 31, 2018