test: add RestQueryTranslationTest #3291

big-andy-coates · 2019-09-02T17:00:31Z

Description

This is along the same lines as QueryTranslationTest, but comes in through the front door of the REST Api.

It's currently much more limited in its functionality, but can extended as needed to be it more inline with QTT functionality.

QTT should still do the brunt of the work at it runs ~ 5 times quicker, with RQTT only being used where QTT will no suffice, e.g. INSERT VALUE statement test, or the upcoming static query work.

I've added an example test in rest-query-translation-tests/insert-values.json

This PR also includes a change to QTT: if the expected output record does not have timestamp defined then the timestamp of the output is not checked. Before this PR you could omit timestamp from both input and output records, but this just set it to zero. This is still the case for input records. This change means system generated timestamps, e.g. like those generated by some INSERT VALUE statements don't mean you can't test it with (R)QTT.

Reviewing notes:

I've lift and shifted a load of code out of QueryTranslationTest and EndToEndIntegrationTestUtil into some new TestLoaders. JsonTestLoader loads JSON based tests from a directory, and ExpectedTopologiesTestLoader which deals with combining the loaded JSON tests with their expected-topology files, and into the existing TopologyFileGenerator. NOTE This code generally hasn't be refactored, just moved - please refrain from raising loads of changes in this code. The purpose of this PR is not to refactor all this code, just to restructure it so it can be shared.
Likewise, the computation has been pulled out of TestCase and into the TestExecutor class, leaving TestCase as a Pojo. NOTE: again, this is a pick and shift of code: please refrain from reviewing line-by-line. We know this code needs improvement / refactoring.
There is a new VersionedTest, which extends the current Test interface with the bits ExpectedTopologiesTestLoader needs to do its work.
Many bits of code have been changed to reflect the fact that the input/output Record might not have an explicit timestamp.
I've added some functionality to embedded kafka code to allow us to delete all the topics, except the command topic.

Testing done

mvn test.

Reviewer checklist

Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
Ensure relevant issues are linked (description should include text like "Fixes #")

This is along the same lines as `QueryTranslationTest`, but comes in through the front door of the REST Api. It's currently much more limited in its functionality, but can extended as needed to be it more inline with QTT functionality. QTT should still do the brunt of the work, with RQTT only being used where QTT will no suffice, e.g. `INSERT VALUE` statement test, or the upcoming static query work.

To allow TestCaseNode to be reused by other computation, split the computation out and leave a Pojo.

rodesai

I've had a pass through. Can we add a readme or doc-string somewhere explaining when to use RQTT instead of QTT? Would it be possible to automate a warning - e.g. if we see a test case with no insert-into or iq usage?

ksql-functional-tests/src/main/java/io/confluent/ksql/test/tools/TestCaseBuilderUtil.java

rodesai · 2019-09-04T02:43:33Z

ksql-functional-tests/src/main/java/io/confluent/ksql/test/tools/TestCaseBuilderUtil.java

+  ) {
+    final String format = explicitFormat.orElse(FORMAT_REPLACE_ERROR);
+
+    return statements.stream()


This will cause a parse error down the line if there is no format, right? Consider just checking for {FORMAT} and throwing here if no format is provided.

rodesai · 2019-09-04T02:46:33Z

ksql-functional-tests/src/main/java/io/confluent/ksql/test/tools/TestCaseBuilderUtil.java

+        .collect(Collectors.toList());
+  }
+
+  public static Map<String, Topic> getTopicsByName(


A doc-string would help here

Again, this is just moved code. I'm trying to move fast here. Sure, a lot of this code could do with refactoring, docs, tests, etc - but I see this PR as moving in the right direction, not the final state.

The old code had no docs, hence the moved code remains with no docs

ksql-functional-tests/src/main/java/io/confluent/ksql/test/model/TopicNode.java

ksql-functional-tests/src/test/java/io/confluent/ksql/test/rest/model/ExpectedErrorNode.java

rodesai · 2019-09-04T03:25:21Z

ksql-functional-tests/src/test/java/io/confluent/ksql/test/rest/RestTestExecutor.java

+    });
+  }
+
+  private static void verifyResponses(


Do we expect the exact response (as specified in the test spec) here? If so, should we run with EOS to try and avoid flakiness? Alternatively, we could loosen our checks below to allow for at-least-once results.

This is something I've considered. I don't think loosening our checks is sufficient, as without EOS aggregates can get the wrong value.

Let's see how we go and enable EOS if we think its needed.

ksql-functional-tests/src/test/java/io/confluent/ksql/test/rest/RestTestExecutor.java

rodesai · 2019-09-04T03:32:33Z

ksql-functional-tests/src/main/java/io/confluent/ksql/test/tools/TestExecutor.java

+        ));
+  }
+
+  private static void throwIfMoreOutputAvailable(


This isn't used anywhere. Should it get called above?

ah, no. Unfortunately, @hjafarpour dropped this functionality in the new code paths :(

All I can do in this PR is remove the unused method :'(

rodesai · 2019-09-04T03:48:09Z

ksql-functional-tests/src/main/java/io/confluent/ksql/test/tools/TestExecutor.java

+
+      testCase.expectedException().map(ee -> {
+        throw new AssertionError("Expected test to throw" + StringDescription.toString(ee));
+      });

      writeInputIntoTopics(testCase.getInputRecords(), fakeKafkaService);


Not part of your diff but this seems weird. We write all the input records to the kafka fake, but they are never read from there. Later in this method we just write these directly into the test driver.

God knows - this code is a mess. But outside the scope of this PR.

rodesai · 2019-09-04T03:59:52Z

ksql-functional-tests/src/test/java/io/confluent/ksql/test/TopologyFileGenerator.java

+                .getSource(persistentQueryMetadata.getSinkName())
+                .getKafkaTopicName();
+
+            final SerdeSupplier<?> keySerdes = SerdeUtil.getKeySerdeSupplier(


Is this all just validation? Would help to add a comment explaining why we need to build all this stuff (and seemingly drop it on the floor)

This is not new code, just moved - as I said in the description, please refrain from asking for improvements in this code. I agree it needs improvement, but this is outside the scope of this PR.

rodesai

LGTM

big-andy-coates requested a review from a team as a code owner September 2, 2019 17:00

big-andy-coates added 3 commits September 2, 2019 18:01

chore: missed change

ef2943f

refactor: split TestCaseNode computation from data

a8bb1a8

To allow TestCaseNode to be reused by other computation, split the computation out and leave a Pojo.

feat: support checking of response and expected errors

5fab5c6

rodesai reviewed Sep 4, 2019

View reviewed changes

Merge branch 'master' into rest_qtt

b193573

big-andy-coates requested review from rodesai and a team September 4, 2019 09:56

big-andy-coates added 3 commits September 4, 2019 11:00

chore: rohan's requested changes

3b584ac

chore: fix test

556eb10

chore: fix test

fa61569

rodesai approved these changes Sep 4, 2019

View reviewed changes

chore: fix test

c6ddddf

big-andy-coates merged commit 26af8d0 into confluentinc:master Sep 6, 2019

big-andy-coates deleted the rest_qtt branch September 6, 2019 07:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: add RestQueryTranslationTest #3291

test: add RestQueryTranslationTest #3291

big-andy-coates commented Sep 2, 2019 •

edited

Loading

rodesai left a comment

rodesai Sep 4, 2019

rodesai Sep 4, 2019

big-andy-coates Sep 4, 2019

rodesai Sep 4, 2019

big-andy-coates Sep 4, 2019

rodesai Sep 4, 2019

big-andy-coates Sep 4, 2019

big-andy-coates Sep 4, 2019

rodesai Sep 4, 2019

big-andy-coates Sep 4, 2019

rodesai Sep 4, 2019

big-andy-coates Sep 4, 2019

rodesai left a comment

test: add RestQueryTranslationTest #3291

test: add RestQueryTranslationTest #3291

Conversation

big-andy-coates commented Sep 2, 2019 • edited Loading

Description

Reviewing notes:

Testing done

Reviewer checklist

rodesai 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

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

rodesai left a comment

Choose a reason for hiding this comment

big-andy-coates commented Sep 2, 2019 •

edited

Loading