Skip to content

Commit

Permalink
jarek/6736: [magic command] allow spaces in jar names (#6797)
Browse files Browse the repository at this point in the history 
* #6736: handle jars with spaces

* #6736: handle exceptions
  • Loading branch information
@jaroslawmalekcodete @scottdraves
jaroslawmalekcodete authored and scottdraves committed Feb 7, 2018
1 parent 01aa5fe commit 5a9fa84
Show file tree
Hide file tree
Showing 9 changed files with 137 additions and 63 deletions.
1 change: 0 additions & 1 deletion kernel/base/src/main/java/com/twosigma/beakerx/kernel/PathToJar.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ public class PathToJar {

public PathToJar(final String path) {
checkNotNull(path);
checkNotWhitespaces(path);
File file = getFile(path);
try {
canonicalPath = file.getCanonicalPath();
Expand Down
15 changes: 14 additions & 1 deletion kernel/base/src/main/java/com/twosigma/beakerx/kernel/handler/ExecuteRequestHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@


import com.twosigma.beakerx.handler.KernelHandler;
import com.twosigma.beakerx.jvm.object.SimpleEvaluationObject;
import com.twosigma.beakerx.kernel.Code;
import com.twosigma.beakerx.kernel.KernelFunctionality;
import com.twosigma.beakerx.kernel.magic.command.CodeFactory;
Expand All @@ -29,6 +30,7 @@
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;

import static com.twosigma.beakerx.kernel.PlainCode.createSimpleEvaluationObject;
import static com.twosigma.beakerx.kernel.msg.JupyterMessages.EXECUTE_INPUT;
import static java.util.Collections.singletonList;

Expand All @@ -49,7 +51,13 @@ public ExecuteRequestHandler(KernelFunctionality kernel) {

@Override
public void handle(Message message) {
executorService.submit(() -> handleMsg(message));
executorService.submit(() -> {
try {
handleMsg(message);
} catch (Exception e) {
handleException(message, e);
}
});
}

private void handleMsg(Message message) {
Expand Down Expand Up @@ -86,6 +94,11 @@ private void announceTheCode(Message message, String code) {
kernel.publish(singletonList(reply));
}

private void handleException(Message message, Exception e) {
SimpleEvaluationObject seo = createSimpleEvaluationObject(takeCodeFrom(message), kernel, message, executionCount);
seo.error(e);
}

@Override
public void exit() {
}
Expand Down
13 changes: 12 additions & 1 deletion kernel/base/src/test/java/com/twosigma/beakerx/KernelTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,13 @@
import com.twosigma.beakerx.kernel.Repos;
import com.twosigma.beakerx.kernel.comm.Comm;
import com.twosigma.beakerx.kernel.magic.command.MagicCommandType;
import com.twosigma.beakerx.kernel.magic.command.MagicCommandWhichThrowsException;
import com.twosigma.beakerx.kernel.magic.command.MavenJarResolver;
import com.twosigma.beakerx.kernel.magic.command.functionality.AddImportMagicCommand;
import com.twosigma.beakerx.kernel.magic.command.functionality.AddStaticImportMagicCommand;
import com.twosigma.beakerx.kernel.magic.command.functionality.BashMagicCommand;
import com.twosigma.beakerx.kernel.magic.command.functionality.ClassPathAddMvnCellMagicCommand;
import com.twosigma.beakerx.kernel.magic.command.functionality.ClasspathAddDynamicMagicCommand;
import com.twosigma.beakerx.kernel.magic.command.functionality.ClasspathAddJarMagicCommand;
import com.twosigma.beakerx.kernel.magic.command.functionality.ClasspathAddMvnMagicCommand;
import com.twosigma.beakerx.kernel.magic.command.functionality.ClasspathAddRepoMagicCommand;
Expand Down Expand Up @@ -73,7 +75,6 @@

import static com.twosigma.beakerx.kernel.magic.command.ClasspathAddMvnDepsMagicCommandTest.TEST_MVN_CACHE;
import static java.util.Arrays.asList;
import static java.util.Collections.unmodifiableList;
import static java.util.Collections.synchronizedList;

public class KernelTest implements KernelFunctionality {
Expand Down Expand Up @@ -134,6 +135,8 @@ private void initMagicCommands() {
new ClasspathAddMvnMagicCommand(mavenResolverParam, this)),
new MagicCommandType(ClassPathAddMvnCellMagicCommand.CLASSPATH_ADD_MVN_CELL, "<group name version>",
new ClassPathAddMvnCellMagicCommand(mavenResolverParam, this)),
addDynamic(this),
addMagicCommandWhichThrowsException(),
new MagicCommandType(ClasspathRemoveMagicCommand.CLASSPATH_REMOVE, "<jar path>", new ClasspathRemoveMagicCommand(this)),
new MagicCommandType(ClasspathShowMagicCommand.CLASSPATH_SHOW, "", new ClasspathShowMagicCommand(this)),
new MagicCommandType(AddStaticImportMagicCommand.ADD_STATIC_IMPORT, "<classpath>", new AddStaticImportMagicCommand(this)),
Expand All @@ -147,6 +150,14 @@ private void initMagicCommands() {
));
}

private static MagicCommandType addDynamic(KernelFunctionality kernel) {
return new MagicCommandType(ClasspathAddDynamicMagicCommand.CLASSPATH_ADD_DYNAMIC, "", new ClasspathAddDynamicMagicCommand(kernel));
}

private static MagicCommandType addMagicCommandWhichThrowsException() {
return new MagicCommandType(MagicCommandWhichThrowsException.MAGIC_COMMAND_WHICH_THROWS_EXCEPTION, "", new MagicCommandWhichThrowsException());
}

@Override
public void publish(List<Message> message) {
this.publishedMessages.addAll(message);
Expand Down
23 changes: 15 additions & 8 deletions kernel/base/src/test/java/com/twosigma/beakerx/evaluator/EvaluatorResultTestWatcher.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,17 +20,13 @@
import com.twosigma.beakerx.KernelTest;
import com.twosigma.beakerx.jupyter.SearchMessages;
import com.twosigma.beakerx.kernel.msg.JupyterMessages;
import com.twosigma.beakerx.jvm.object.SimpleEvaluationObject;
import com.twosigma.beakerx.message.Message;
import com.twosigma.beakerx.widgets.TestWidgetUtils;

import java.util.List;
import java.util.Optional;
import java.util.stream.Collectors;

import static com.twosigma.beakerx.jvm.object.SimpleEvaluationObject.EvaluationStatus.QUEUED;
import static com.twosigma.beakerx.jvm.object.SimpleEvaluationObject.EvaluationStatus.RUNNING;

public class EvaluatorResultTestWatcher {

public static final int ATTEMPT = 2000;
Expand Down Expand Up @@ -91,12 +87,23 @@ public static Optional<Message> waitForStreamMessage(KernelTest kernelTest) thro
return sentMessage;
}

public static Optional<Message> waitForErrorMessage(KernelTest kernelTest) throws InterruptedException {
int count = 0;
Optional<Message> idleMessage = getError(kernelTest.getPublishedMessages());
while (!idleMessage.isPresent() && count < ATTEMPT) {
Thread.sleep(SLEEP_IN_MILLIS);
idleMessage = getError(kernelTest.getPublishedMessages());
count++;
}
return idleMessage;
}

public static Optional<Message> waitForErrorMessage(KernelSocketsTest socketsTest) throws InterruptedException {
int count = 0;
Optional<Message> idleMessage = getError(socketsTest);
Optional<Message> idleMessage = getError(socketsTest.getPublishedMessages());
while (!idleMessage.isPresent() && count < ATTEMPT) {
Thread.sleep(SLEEP_IN_MILLIS);
idleMessage = getError(socketsTest);
idleMessage = getError(socketsTest.getPublishedMessages());
count++;
}
return idleMessage;
Expand Down Expand Up @@ -145,8 +152,8 @@ private static Optional<Message> getResult(KernelSocketsTest socketsTest) {
filter(x -> x.type().equals(JupyterMessages.EXECUTE_RESULT)).findFirst();
}

private static Optional<Message> getError(KernelSocketsTest socketsTest) {
return socketsTest.getPublishedMessages().stream().
private static Optional<Message> getError(List<Message> messages) {
return messages.stream().
filter(x -> x.type().equals(JupyterMessages.ERROR)).findFirst();
}

Expand Down
91 changes: 52 additions & 39 deletions ...el/base/src/test/java/com/twosigma/beakerx/jupyter/handler/ExecuteRequestHandlerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,29 +16,33 @@

package com.twosigma.beakerx.jupyter.handler;

import static com.twosigma.beakerx.evaluator.EvaluatorResultTestWatcher.waitForIdleMessage;
import static com.twosigma.beakerx.kernel.msg.JupyterMessages.EXECUTE_REPLY;
import static com.twosigma.beakerx.message.MessageSerializer.parse;
import static com.twosigma.beakerx.message.MessageSerializer.toJson;

import com.twosigma.beakerx.KernelTest;
import com.twosigma.beakerx.evaluator.EvaluatorTest;
import com.twosigma.beakerx.kernel.handler.ExecuteRequestHandler;
import org.assertj.core.api.Assertions;
import com.twosigma.beakerx.kernel.magic.command.MagicCommandWhichThrowsException;
import com.twosigma.beakerx.kernel.msg.JupyterMessages;
import com.twosigma.beakerx.message.Header;
import com.twosigma.beakerx.message.Message;
import org.junit.After;
import org.junit.AfterClass;
import org.junit.Before;
import org.junit.BeforeClass;
import org.junit.Test;

import com.twosigma.beakerx.KernelTest;
import com.twosigma.beakerx.evaluator.EvaluatorTest;
import com.twosigma.beakerx.kernel.msg.JupyterMessages;
import com.twosigma.beakerx.message.Header;
import com.twosigma.beakerx.message.Message;

import java.util.LinkedHashMap;
import java.util.List;
import java.util.Optional;
import java.util.stream.Collectors;

import static com.twosigma.beakerx.evaluator.EvaluatorResultTestWatcher.waitForErrorMessage;
import static com.twosigma.beakerx.evaluator.EvaluatorResultTestWatcher.waitForIdleMessage;
import static com.twosigma.beakerx.kernel.msg.JupyterMessages.EXECUTE_REPLY;
import static com.twosigma.beakerx.message.MessageSerializer.parse;
import static com.twosigma.beakerx.message.MessageSerializer.toJson;
import static org.assertj.core.api.Assertions.assertThat;



public class ExecuteRequestHandlerTest {

private static KernelTest kernel;
Expand Down Expand Up @@ -82,9 +86,9 @@ public void handleMessage_firstSentMessageHasExecutionStateIsBusy() throws Excep
executeRequestHandler.handle(message);
waitForIdleMessage(kernel);
//then
Assertions.assertThat(kernel.getPublishedMessages()).isNotEmpty();
assertThat(kernel.getPublishedMessages()).isNotEmpty();
Message publishMessage = kernel.getPublishedMessages().get(0);
Assertions.assertThat(publishMessage.getContent().get("execution_state")).isEqualTo("busy");
assertThat(publishMessage.getContent().get("execution_state")).isEqualTo("busy");
}

@Test
Expand All @@ -95,9 +99,9 @@ public void handleMessage_firstSentMessageHasSessionId() throws Exception {
executeRequestHandler.handle(message);
waitForIdleMessage(kernel);
//then
Assertions.assertThat(kernel.getPublishedMessages()).isNotEmpty();
assertThat(kernel.getPublishedMessages()).isNotEmpty();
Message publishMessage = kernel.getPublishedMessages().get(0);
Assertions.assertThat(publishMessage.getHeader().getSession()).isEqualTo(expectedSessionId);
assertThat(publishMessage.getHeader().getSession()).isEqualTo(expectedSessionId);
}

@Test
Expand All @@ -106,9 +110,9 @@ public void handleMessage_firstSentMessageHasTypeIsStatus() throws Exception {
executeRequestHandler.handle(message);
waitForIdleMessage(kernel);
//then
Assertions.assertThat(kernel.getPublishedMessages()).isNotEmpty();
assertThat(kernel.getPublishedMessages()).isNotEmpty();
Message publishMessage = kernel.getPublishedMessages().get(0);
Assertions.assertThat(publishMessage.getHeader().getType())
assertThat(publishMessage.getHeader().getType())
.isEqualTo(JupyterMessages.STATUS.getName());
}

Expand All @@ -120,9 +124,9 @@ public void handleMessage_firstSentMessageHasParentHeader() throws Exception {
executeRequestHandler.handle(message);
waitForIdleMessage(kernel);
//then
Assertions.assertThat(kernel.getPublishedMessages()).isNotEmpty();
assertThat(kernel.getPublishedMessages()).isNotEmpty();
Message publishMessage = kernel.getPublishedMessages().get(0);
Assertions.assertThat(publishMessage.getParentHeader().asJson()).isEqualTo(expectedHeader);
assertThat(publishMessage.getParentHeader().asJson()).isEqualTo(expectedHeader);
}

@Test
Expand All @@ -133,9 +137,9 @@ public void handleMessage_firstSentMessageHasIdentities() throws Exception {
executeRequestHandler.handle(message);
waitForIdleMessage(kernel);
//then
Assertions.assertThat(kernel.getPublishedMessages()).isNotEmpty();
assertThat(kernel.getPublishedMessages()).isNotEmpty();
Message publishMessage = kernel.getPublishedMessages().get(0);
Assertions.assertThat(new String(publishMessage.getIdentities().get(0)))
assertThat(new String(publishMessage.getIdentities().get(0)))
.isEqualTo(expectedIdentities);
}

Expand All @@ -147,9 +151,9 @@ public void handleMessage_secondSentMessageHasSessionId() throws Exception {
executeRequestHandler.handle(message);
waitForIdleMessage(kernel);
//then
Assertions.assertThat(kernel.getPublishedMessages()).isNotEmpty();
assertThat(kernel.getPublishedMessages()).isNotEmpty();
Message publishMessage = kernel.getPublishedMessages().get(1);
Assertions.assertThat(publishMessage.getHeader().getSession()).isEqualTo(expectedSessionId);
assertThat(publishMessage.getHeader().getSession()).isEqualTo(expectedSessionId);
}

@Test
Expand All @@ -158,9 +162,9 @@ public void handleMessage_secondSendMessageHasTypeIsExecutionInput() throws Exce
executeRequestHandler.handle(message);
waitForIdleMessage(kernel);
//then
Assertions.assertThat(kernel.getPublishedMessages()).isNotEmpty();
assertThat(kernel.getPublishedMessages()).isNotEmpty();
Message publishMessage = kernel.getPublishedMessages().get(1);
Assertions.assertThat(publishMessage.getHeader().getType())
assertThat(publishMessage.getHeader().getType())
.isEqualTo(JupyterMessages.EXECUTE_INPUT.getName());
}

Expand All @@ -172,9 +176,9 @@ public void handleMessage_secondSentMessageHasContentCode() throws Exception {
executeRequestHandler.handle(message);
waitForIdleMessage(kernel);
//then
Assertions.assertThat(kernel.getPublishedMessages()).isNotEmpty();
assertThat(kernel.getPublishedMessages()).isNotEmpty();
Message publishMessage = kernel.getPublishedMessages().get(1);
Assertions.assertThat(publishMessage.getContent().get("code")).isEqualTo(expectedCode);
assertThat(publishMessage.getContent().get("code")).isEqualTo(expectedCode);
}

@Test
Expand All @@ -183,9 +187,9 @@ public void handleMessage_secondSentMessageHasContentExecutionCount() throws Exc
executeRequestHandler.handle(message);
waitForIdleMessage(kernel);
//then
Assertions.assertThat(kernel.getPublishedMessages()).isNotEmpty();
assertThat(kernel.getPublishedMessages()).isNotEmpty();
Message publishMessage = kernel.getPublishedMessages().get(1);
Assertions.assertThat(publishMessage.getContent().get("execution_count")).isNotNull();
assertThat(publishMessage.getContent().get("execution_count")).isNotNull();
}

@Test
Expand All @@ -196,9 +200,9 @@ public void handleMessage_secondSentMessageHasParentHeader() throws Exception {
executeRequestHandler.handle(message);
waitForIdleMessage(kernel);
//then
Assertions.assertThat(kernel.getPublishedMessages()).isNotEmpty();
assertThat(kernel.getPublishedMessages()).isNotEmpty();
Message publishMessage = kernel.getPublishedMessages().get(1);
Assertions.assertThat(publishMessage.getParentHeader().asJson()).isEqualTo(expectedHeader);
assertThat(publishMessage.getParentHeader().asJson()).isEqualTo(expectedHeader);
}

@Test
Expand All @@ -209,9 +213,9 @@ public void handleMessage_secondSentMessageHasIdentities() throws Exception {
executeRequestHandler.handle(message);
waitForIdleMessage(kernel);
//then
Assertions.assertThat(kernel.getPublishedMessages()).isNotEmpty();
assertThat(kernel.getPublishedMessages()).isNotEmpty();
Message publishMessage = kernel.getPublishedMessages().get(1);
Assertions.assertThat(new String(publishMessage.getIdentities().get(0)))
assertThat(new String(publishMessage.getIdentities().get(0)))
.isEqualTo(expectedIdentities);
}

Expand All @@ -222,11 +226,11 @@ public void handleMagicMessage_executionStateStartsBusyEndsIdle() throws Excepti
waitForIdleMessage(kernel);
//then
final List<Message> publishedMessages = kernel.getPublishedMessages();
Assertions.assertThat(publishedMessages).isNotEmpty();
assertThat(publishedMessages).isNotEmpty();
Message firstPublishedMessage = publishedMessages.get(0);
Assertions.assertThat(firstPublishedMessage.getContent().get("execution_state")).isEqualTo("busy");
assertThat(firstPublishedMessage.getContent().get("execution_state")).isEqualTo("busy");
Message lastPublishedMessage = publishedMessages.get(publishedMessages.size() - 1);
Assertions.assertThat(lastPublishedMessage.getContent().get("execution_state")).isEqualTo("idle");
assertThat(lastPublishedMessage.getContent().get("execution_state")).isEqualTo("idle");
}

@Test
Expand All @@ -236,9 +240,9 @@ public void handleMagicMessage_replyIsSent() throws InterruptedException {
waitForIdleMessage(kernel);
//then
final List<Message> sentMessages = kernel.getSentMessages();
Assertions.assertThat(sentMessages).isNotEmpty();
assertThat(sentMessages).isNotEmpty();
Message firstSentMessage = sentMessages.get(0);
Assertions.assertThat(firstSentMessage.getHeader().getTypeEnum()).isEqualTo(EXECUTE_REPLY);
assertThat(firstSentMessage.getHeader().getTypeEnum()).isEqualTo(EXECUTE_REPLY);
}

private static Message copyMessage(Message origin) {
Expand All @@ -256,4 +260,13 @@ private static Message copyMessage(Message origin) {
copy.setContent(parse(content, LinkedHashMap.class));
return copy;
}

@Test
public void shouldSendErrorMessageWhenMagicCommandThrowsException() throws InterruptedException {
magicMessage = JupyterHandlerTest.initExecuteRequestMessage();
magicMessage.getContent().put("code", MagicCommandWhichThrowsException.MAGIC_COMMAND_WHICH_THROWS_EXCEPTION);
executeRequestHandler.handle(magicMessage);
Optional<Message> message = waitForErrorMessage(kernel);
assertThat(message).isPresent();
}
}
13 changes: 0 additions & 13 deletions kernel/base/src/test/java/com/twosigma/beakerx/kernel/PathToJarTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,17 +46,4 @@ public void shouldThrowErrorIfPathDoesNotExist() throws Exception {
Assertions.assertThat(e.getMessage()).contains("Path does not exist");
}
}

@Test
public void shouldThrowErrorIfPathContainsWhitespace() throws Exception {
//given
//when
try {
new PathToJar("./pathWithWhitespace demo.jar");
fail("Should not create PathToJar with whitespace.");
} catch (Exception e) {
//then
Assertions.assertThat(e.getMessage()).contains("Can not create path with whitespace");
}
}
}
Loading

0 comments on commit 5a9fa84

Please sign in to comment.