Bring back the commented assertions in StatementExecutorTest and aCom… #406

hjafarpour · 2017-10-23T22:05:59Z

…mandRunnerTest
The problem with the previous test was that it was trying to connect to a kafka broker and since there was no brokers available it would fail for both cases, correct and incorrect queries. The assertions were also set incorrectly and for the correct query the expected result was ERROR.
In order to have the test behave correctly for both correct and incorrect queries we now use an embedded test broker similar to the tests in the engine. This need dependencies such as kafka and curator.

…mandRunnerTest

apurvam · 2017-10-24T00:33:49Z

@hjafarpour are you planning on fixing the failures caused by these assertions?

hjafarpour · 2017-10-24T04:20:33Z

@apurvam yes, PR is ready for review.

apurvam · 2017-10-24T05:44:54Z

Thanks @hjafarpour. Could you add a note about problems which caused the failures and how you fixed them in the description of the PR?

apurvam · 2017-10-24T05:45:30Z

ksql-rest-app/pom.xml

@@ -51,6 +52,38 @@
            <artifactId>junit</artifactId>
            <scope>test</scope>
        </dependency>
+        <dependency>
+            <groupId>org.apache.curator</groupId>


Why do we need this dependency on curator? As mentioned before, a description of the problem and the fix would help in understanding this decision.

@apurvam added more info on the problem in the description of the PR.

I still don't get why we need curator? I don't believe that EmbeddedSingleNodeKafkaCluster uses it. It would need a zookeeper dependency

EmbeddedSingleNodeKafkaCluster uses ZooKeeperEmbedded which depends on curator.
Here I'm using t he same implementation for EmbeddedSingleNodeKafkaCluster as the one in streams-example. Do we have any other implementation?

dguy · 2017-10-24T09:05:01Z

ksql-rest-app/pom.xml

@@ -51,6 +52,38 @@
            <artifactId>junit</artifactId>
            <scope>test</scope>
        </dependency>
+        <dependency>
+            <groupId>org.apache.curator</groupId>


I still don't get why we need curator? I don't believe that EmbeddedSingleNodeKafkaCluster uses it. It would need a zookeeper dependency

dguy · 2017-10-24T09:07:22Z

ksql-rest-app/src/test/java/io/confluent/ksql/rest/server/computation/CommandRunnerTest.java

+
+  @AfterClass
+  public static void cleanUp() {
+    CLUSTER.stop();


Don't need this as it happens automatically when you use @ClassRule

dguy · 2017-10-24T09:10:02Z

ksql-rest-app/src/test/java/io/confluent/ksql/rest/server/computation/CommandRunnerTest.java


 public class CommandRunnerTest {

+  @ClassRule
+  public static final EmbeddedSingleNodeKafkaCluster CLUSTER = new EmbeddedSingleNodeKafkaCluster();


Why does this need an embedded broker? Could we not just use the MockKafkaConsumer etc?

Refactored this test to mock requires objects instead of using EmbeddedSingleNodeKafkaCluster.

dguy · 2017-10-24T09:14:46Z

ksql-rest-app/src/test/java/io/confluent/ksql/rest/server/computation/CommandRunnerTest.java

+  }
+
+  @Test
+  public void testThread() throws InterruptedException {


What is this testing?

Renamed the method for better description.

dguy · 2017-10-24T09:18:05Z

ksql-rest-app/src/test/java/io/confluent/ksql/rest/server/computation/CommandRunnerTest.java

    CommandRunner commandRunner = getCommanRunner(statementExecutor);
    new Thread(commandRunner).start();
-    Thread.sleep(50000);
+    Thread.sleep(2000);


We should be avoiding Thread.sleep(...) in tests. With the code the way it currently is, you could pass a mock/stub version of the StatementExectuor to the CommandRunner and use a latch or counter to determine when it has received the expected commands.

Refactored this test along with the CommandRunner test so we won't need to test the thread.

dguy · 2017-10-24T09:19:32Z

ksql-rest-app/src/test/java/io/confluent/ksql/rest/server/computation/CommandRunnerTest.java

    Assert.assertEquals(CommandStatus.Status.SUCCESS, statusStore.get(topicCommandId).getStatus());
    Assert.assertEquals(CommandStatus.Status.SUCCESS, statusStore.get(csCommandId).getStatus());
-//    Assert.assertEquals(CommandStatus.Status.SUCCESS, statusStore.get(csasCommandId).getStatus());
-//    Assert.assertEquals(CommandStatus.Status.ERROR, statusStore.get(ctasCommandId).getStatus());
+    if (statusStore.containsKey(csasCommandId)) {


This seems weird. The test has no control over the commands that are processed. Tests need to be deterministic

Using mocks refactored this so it will be deterministic.

…e thread to a new method for better testablility.

apurvam

Thanks for the updates Hojjat. I think this is starting to look good.

These changes have made me curious though: from what you wrote in the description, these tests should always have failed, independently of #391 . Is this true?

apurvam · 2017-10-25T05:47:23Z

ksql-rest-app/src/test/java/io/confluent/ksql/rest/server/computation/CommandRunnerTest.java


 public class CommandRunnerTest {

+  /**
+   *   Get a command runner instance using mock values
+    */
  private CommandRunner getCommanRunner(StatementExecutor statementExecutor) {


nit: we have a typo here

Good catch, fixed it.

apurvam · 2017-10-25T05:49:12Z

...-rest-app/src/test/java/io/confluent/ksql/rest/server/computation/StatementExecutorTest.java

 import io.confluent.ksql.util.Pair;

 public class StatementExecutorTest extends EasyMockSupport {

+  @ClassRule
+  public static final EmbeddedSingleNodeKafkaCluster CLUSTER = new EmbeddedSingleNodeKafkaCluster();


So to be clear, this line is the only reason we have included the dependency on Curator? Can't you use mocks here just like you did in your updates to the other test?

Indeed I tried that first by trying to mock KsqlEngine class but this class in addition to its internal QueryEngine and Metastore instances are being used in several places and mocking them would be much more complex for multiple statements. Instead I opted to use the embedded broker.
Also when we add more integration tests for this module we will need Embedded broker there too.

apurvam · 2017-10-25T05:50:00Z

...-rest-app/src/test/java/io/confluent/ksql/rest/server/computation/StatementExecutorTest.java

+    Map<String, Object> props = new HashMap<>();
+    props.put("application.id", "ksqlStatementExecutorTest");
+    props.put("bootstrap.servers", CLUSTER.bootstrapServers());
+
    MockKsqkEngine mockKsqkEngine = new MockKsqkEngine(


There is a typo on the class and variable names. MockKsqkEngine should be MockKsqlEngine.

This class does not add any functionality, I deleted it and used KsqlEngine instead.

dguy · 2017-10-25T09:31:54Z

ksql-rest-app/src/test/java/io/confluent/ksql/rest/server/computation/CommandRunnerTest.java

 import io.confluent.ksql.rest.server.utils.TestUtils;
+import io.confluent.ksql.util.Pair;
+
+import static org.easymock.EasyMock.*;


You probably need to change your import settings. We don't really want to be importing *

Changed it. Need to see why my IntelliJ didn't catch this!

dguy · 2017-10-25T09:38:36Z

ksql-rest-app/src/main/java/io/confluent/ksql/rest/server/computation/CommandRunner.java

@@ -91,6 +81,20 @@ public void close() {
    commandStore.close();
  }

+  public void fetchAndRunCommands() {


This method is only visible due to testing. At the very least it should be made package-private, i.e, it def shouldn't be public.
An alternative would be to not make this class Runnable and instead use an ExecutorService in KsqlRestApplication to submit a Runnable with a fixed delay. You would then keep this public method on CommandRunner and remove the run method with the while loop.

Made it package-private.

dguy · 2017-10-25T09:39:33Z

ksql-rest-app/src/main/java/io/confluent/ksql/rest/server/computation/CommandRunner.java

@@ -91,6 +81,20 @@ public void close() {
    commandStore.close();
  }

+  public void fetchAndRunCommands() {
+    ConsumerRecords<CommandId, Command> records = commandStore.getNewCommands();
+    log.debug("Found {} new writes to command topic", records.count());


Maybe this message should be trace level? It has potential to be quite spammy

Good point, changed it to trace.

dguy · 2017-10-25T09:40:10Z

ksql-rest-app/src/main/java/io/confluent/ksql/rest/server/computation/CommandRunner.java

+      if (command.getStatement() != null) {
+        executeStatement(command, commandId);
+      } else {
+        log.debug("Skipping null statement for ID {}", commandId);


In what circumstance does this happen?

Technically we should not have any command with null value since KSQL should be the only one who writes the commands to the command topic. I guess this remained here when our intern was refactoring. I'll remove it.

dguy · 2017-10-25T09:44:43Z

ksql-rest-app/src/test/java/io/confluent/ksql/rest/server/computation/CommandRunnerTest.java

+    List<Pair<CommandId, Command>> commandList = new TestUtils().getAllPriorCommandRecords();
+    List<ConsumerRecord<CommandId, Command>> recordList = new ArrayList<>();
+    for (Pair commandPair: commandList) {
+      recordList.add(new ConsumerRecord<CommandId, Command>("T", 1, 1, (CommandId) commandPair


nit: redundant '<CommandId, Command>`

Removed it.

dguy · 2017-10-25T09:53:53Z

ksql-rest-app/src/test/java/io/confluent/ksql/rest/server/computation/CommandRunnerTest.java

 import io.confluent.ksql.rest.server.utils.TestUtils;
+import io.confluent.ksql.util.Pair;
+
+import static org.easymock.EasyMock.*;

 public class CommandRunnerTest {


Needs a test for when a statement is null, i.e., StatementExecutor#handleStatements should not be called

Added the suggested test.

dguy · 2017-10-25T09:56:04Z

ksql-rest-app/src/test/java/io/confluent/ksql/rest/server/computation/CommandRunnerTest.java

-        TestUtils.getMockKsqlConfig(), new MockKafkaTopicClient());
-    StatementParser statementParser = new StatementParser(mockKsqkEngine);
-    StatementExecutor statementExecutor = new StatementExecutor(mockKsqkEngine, statementParser);
+  public void testNewCommandRun() throws Exception {


what does the test do? It is easier to understand what should happen if we have good test names, i.e, "shouldExecuteNewCommandsFromCommandStore"

renamed the tests.

dguy · 2017-10-25T09:56:22Z

ksql-rest-app/src/test/java/io/confluent/ksql/rest/server/computation/CommandRunnerTest.java

-//    Assert.assertEquals(CommandStatus.Status.ERROR, statusStore.get(ctasCommandId).getStatus());
+    Assert.assertEquals(CommandStatus.Status.SUCCESS, statusStore.get(csasCommandId).getStatus());
+    Assert.assertEquals(CommandStatus.Status.ERROR, statusStore.get(ctasCommandId).getStatus());
+    verify(statementExecutor);
  }

  @Test
  public void testPriorCommandsRun() throws Exception {


same here: what "should" it do?

changed the name.

dguy · 2017-10-25T09:58:39Z

...-rest-app/src/test/java/io/confluent/ksql/rest/server/computation/StatementExecutorTest.java

@@ -114,11 +122,12 @@ public void handleCSAS_CTASStatement() throws Exception {

    Map<CommandId, CommandStatus> statusStore = statementExecutor.getStatuses();
    Assert.assertNotNull(statusStore);
+    Assert.assertEquals(6, statusStore.size());


you can create a Map<CommandId, CommandStatus> call it expected and then do:
assertThat(statusStore, equalTo(expected))

The problem is that CommandStatus has two fields, status and message. We only case about the status in this case so if creating CommandStatus with specific message, especially exception message for ERROR case may be much more complex.

dguy · 2017-10-25T09:58:52Z

...-rest-app/src/test/java/io/confluent/ksql/rest/server/computation/StatementExecutorTest.java

@@ -140,8 +149,8 @@ public void handlePriorStatement() throws Exception {
    Assert.assertEquals(4, statusStore.size());
    Assert.assertEquals(CommandStatus.Status.SUCCESS, statusStore.get(topicCommandId).getStatus());


…-assertion-in-CommandRunnerTest-StatementExecutorTest

hjafarpour · 2017-10-26T19:47:08Z

@apurvam yes, the tests were not correct previously.

… store. I am reducing it back to 5 seconds and also adding a message in KSQL console to let user know that the service is being initialized.

dguy

A couple of comments otherwise LGTM

dguy · 2017-10-27T13:52:12Z

ksql-rest-app/src/main/java/io/confluent/ksql/rest/server/computation/CommandStore.java

@@ -136,14 +136,14 @@ public CommandId distributeStatement(
        log.debug("Reading prior command records");

        List<ConsumerRecord<CommandId, Command>> result = new ArrayList<>();
-        ConsumerRecords<CommandId, Command> records = commandConsumer.poll(30000);
+        ConsumerRecords<CommandId, Command> records = commandConsumer.poll(5000);


Should we make this configurable rather than hard coding

Or at least extract it to a constant as it is used below, too

@dguy extracted it to a constant.

…-assertion-in-CommandRunnerTest-StatementExecutorTest

hjafarpour · 2017-10-27T16:59:57Z

@dguy applied your latest feedback. @apurvam if you have any more feedback please let me know, otherwise I'll merge this today.

apurvam

LGTM! Thanks!

Bring back the commented assertions in StatementExecutorTest and aCom…

9d0c608

…mandRunnerTest

Fixed the tests.

aad74d8

hjafarpour requested review from dguy and apurvam October 24, 2017 04:20

apurvam reviewed Oct 24, 2017

View reviewed changes

dguy reviewed Oct 24, 2017

View reviewed changes

Refactored tests. Moved command computation code from the run() in th…

921ae42

…e thread to a new method for better testablility.

apurvam reviewed Oct 25, 2017

View reviewed changes

dguy reviewed Oct 25, 2017

View reviewed changes

hjafarpour added 2 commits October 26, 2017 09:35

Merge remote-tracking branch 'upstream/4.0.x' into KSQL-402-Reinstate…

380625d

…-assertion-in-CommandRunnerTest-StatementExecutorTest

Made changes based on the review feedback.

50f881f

While inverstigating the tests I changed the poll time in the command…

2bdb563

… store. I am reducing it back to 5 seconds and also adding a message in KSQL console to let user know that the service is being initialized.

dguy reviewed Oct 27, 2017

View reviewed changes

hjafarpour added 2 commits October 27, 2017 09:29

Extract polling time constant.

ab0fd85

Merge remote-tracking branch 'upstream/4.0.x' into KSQL-402-Reinstate…

6750dc8

…-assertion-in-CommandRunnerTest-StatementExecutorTest

apurvam approved these changes Oct 27, 2017

View reviewed changes

hjafarpour merged commit 0dd8058 into confluentinc:4.0.x Oct 28, 2017

		@@ -140,8 +149,8 @@ public void handlePriorStatement() throws Exception {
		Assert.assertEquals(4, statusStore.size());
		Assert.assertEquals(CommandStatus.Status.SUCCESS, statusStore.get(topicCommandId).getStatus());

Bring back the commented assertions in StatementExecutorTest and aCom… #406

Bring back the commented assertions in StatementExecutorTest and aCom… #406

Conversation

hjafarpour commented Oct 23, 2017 • edited Loading

apurvam commented Oct 24, 2017

hjafarpour commented Oct 24, 2017

apurvam commented Oct 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hjafarpour Oct 24, 2017 • edited Loading

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

apurvam 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

hjafarpour Oct 26, 2017 • edited Loading

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

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

hjafarpour commented Oct 26, 2017

dguy 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

hjafarpour commented Oct 27, 2017

apurvam left a comment

Choose a reason for hiding this comment

hjafarpour commented Oct 23, 2017 •

edited

Loading

hjafarpour Oct 24, 2017 •

edited

Loading

hjafarpour Oct 26, 2017 •

edited

Loading