Skip to content

Commit

Permalink
#6443: fix displaying widgets by fixing parent messages (#6464)
Browse files Browse the repository at this point in the history
  • Loading branch information
@jaroslawmalekcodete @scottdraves
jaroslawmalekcodete authored and scottdraves committed Dec 6, 2017
1 parent 302d896 commit dd34287
Show file tree
Hide file tree
Showing 3 changed files with 86 additions and 48 deletions.
27 changes: 13 additions & 14 deletions kernel/base/src/main/java/com/twosigma/beakerx/kernel/comm/Comm.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,6 @@
import com.twosigma.beakerx.kernel.msg.JupyterMessages;
import com.twosigma.beakerx.message.Header;
import com.twosigma.beakerx.message.Message;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import java.io.Serializable;
import java.util.ArrayList;
Expand All @@ -36,11 +34,8 @@
import static com.twosigma.beakerx.kernel.msg.JupyterMessages.COMM_MSG;
import static com.twosigma.beakerx.kernel.msg.JupyterMessages.COMM_OPEN;


public class Comm {

private static final Logger logger = LoggerFactory.getLogger(Comm.class);

public static final String METHOD = "method";
public static final String UPDATE = "update";
public static final String STATE = "state";
Expand All @@ -62,7 +57,6 @@ public class Comm {
private KernelFunctionality kernel;
private List<Handler<Message>> msgCallbackList = new ArrayList<>();
private List<Handler<Message>> closeCallbackList = new ArrayList<>();
private Message parentMessage;

public Comm(String commId, String targetName) {
super();
Expand Down Expand Up @@ -134,13 +128,12 @@ public void clearCloseCallbackList() {
}

public void open() {
this.parentMessage = InternalVariable.getParentHeader();
doOpen(parentMessage);
doOpen(getParentMessage());
}

public void open(Message parentMessage) {
this.parentMessage = parentMessage;
doOpen(parentMessage);
getParentMessageStrategy = () -> parentMessage;
open();
}

private void doOpen(Message parentMessage) {
Expand All @@ -167,7 +160,7 @@ private void doOpen(Message parentMessage) {
}

public void close() {
Message parentMessage = getParentMessage();// can be null
Message parentMessage = getParentMessage();

if (this.getCloseCallbackList() != null && !this.getCloseCallbackList().isEmpty()) {
for (Handler<Message> handler : getCloseCallbackList()) {
Expand Down Expand Up @@ -225,9 +218,6 @@ public void sendUpdate(final String propertyName, final Object value) {
this.send();
}

private Message getParentMessage() {
return this.parentMessage;
}

public void handleMsg(Message parentMessage) {
for (Handler<Message> handler : this.msgCallbackList) {
Expand All @@ -247,4 +237,13 @@ public String toString() {
return commId + "/" + targetName + "/" + (targetModule != null && !targetModule.isEmpty() ? targetModule : "");
}

public Message getParentMessage() {
return getParentMessageStrategy.getParentMessage();
}

private GetParentMessageStrategy getParentMessageStrategy = InternalVariable::getParentHeader;

interface GetParentMessageStrategy {
Message getParentMessage();
}
}
5 changes: 3 additions & 2 deletions kernel/base/src/main/java/com/twosigma/beakerx/widgets/Widget.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
import static com.twosigma.beakerx.kernel.msg.JupyterMessages.DISPLAY_DATA;
import static com.twosigma.beakerx.widgets.CompiledCodeRunner.runCommEvent;

import com.twosigma.beakerx.evaluator.InternalVariable;
import com.twosigma.beakerx.kernel.KernelManager;
import com.twosigma.beakerx.kernel.comm.Comm;
import com.twosigma.beakerx.kernel.comm.TargetNamesEnum;
Expand Down Expand Up @@ -62,7 +61,9 @@ public Widget() {
}

protected void openComm() {
openComm(InternalVariable.getParentHeader());
comm.setData(createContent());
addValueChangeMsgCallback();
comm.open();
}

protected void openComm(Message parentMessage) {
Expand Down
102 changes: 70 additions & 32 deletions kernel/base/src/test/java/com/twosigma/beakerx/jupyter/comm/CommTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,11 @@
package com.twosigma.beakerx.jupyter.comm;

import com.twosigma.beakerx.KernelTest;
import com.twosigma.beakerx.evaluator.InternalVariable;
import com.twosigma.beakerx.jvm.object.SimpleEvaluationObject;
import com.twosigma.beakerx.kernel.KernelManager;
import com.twosigma.beakerx.kernel.msg.JupyterMessages;
import com.twosigma.beakerx.kernel.comm.Comm;
import org.assertj.core.api.Assertions;
import org.junit.After;
import org.junit.Before;
import com.twosigma.beakerx.message.Message;
Expand All @@ -31,6 +32,8 @@
import java.util.HashMap;
import java.util.Map;

import static org.assertj.core.api.Assertions.assertThat;

public class CommTest {

private KernelTest kernel;
Expand All @@ -48,52 +51,87 @@ public void tearDown() throws Exception {
KernelManager.register(null);
}

@Test
public void commCreatedWithParentMessageShouldAlwaysSendHeaderFromThisParentMessage() throws NoSuchAlgorithmException {
//given
submitCodeToExecution();
//when
Message parentMessage = new Message();
comm.open(parentMessage);
assertThat(kernel.getPublishedMessages().get(0).getParentHeader()).isEqualTo(parentMessage.getHeader());
kernel.clearPublishedMessages();
//then
comm.send();
assertThat(kernel.getPublishedMessages().get(0).getParentHeader()).isEqualTo(parentMessage.getHeader());
}

@Test
public void commCreatedWithoutParentMessageShouldAlwaysSendHeaderFromMessageGivenFromInternalVariable() throws NoSuchAlgorithmException {
// code from first execution
Message message1 = submitCodeToExecution();
comm.open();
assertThat(kernel.getPublishedMessages().get(0).getParentHeader()).isEqualTo(message1.getHeader());
kernel.clearPublishedMessages();
// code from second execution
Message message2 = submitCodeToExecution();
comm.send();
assertThat(kernel.getPublishedMessages().get(0).getParentHeader()).isEqualTo(message2.getHeader());
}

private Message submitCodeToExecution() {
SimpleEvaluationObject value = new SimpleEvaluationObject("ok");
Message jupyterMessage = new Message();
value.setJupyterMessage(jupyterMessage);
InternalVariable.setValue(value);
return jupyterMessage;
}

@Test
public void commOpen_shouldSendIOPubSocketMessage() throws NoSuchAlgorithmException {
//when
comm.open();
//then
Assertions.assertThat(kernel.getPublishedMessages()).isNotEmpty();
Assertions.assertThat(kernel.getPublishedMessages().get(0)).isNotNull();
assertThat(kernel.getPublishedMessages()).isNotEmpty();
assertThat(kernel.getPublishedMessages().get(0)).isNotNull();
}

@Test
public void commOpen_shouldAddCommToStorageMap() throws NoSuchAlgorithmException {
//when
comm.open();
//then
Assertions.assertThat(kernel.isCommPresent(comm.getCommId())).isTrue();
assertThat(kernel.isCommPresent(comm.getCommId())).isTrue();
}

@Test
public void commOpen_sentMessageHasTypeIsCommOpen() throws NoSuchAlgorithmException {
//when
comm.open();
//then
Assertions.assertThat(kernel.getPublishedMessages()).isNotEmpty();
assertThat(kernel.getPublishedMessages()).isNotEmpty();
Message sendMessage = kernel.getPublishedMessages().get(0);
Assertions.assertThat(sendMessage.getHeader().getType())
.isEqualTo(JupyterMessages.COMM_OPEN.getName());
assertThat(sendMessage.getHeader().getType())
.isEqualTo(JupyterMessages.COMM_OPEN.getName());
}

@Test
public void commOpen_sentMessageHasCommId() throws NoSuchAlgorithmException {
//when
comm.open();
//then
Assertions.assertThat(kernel.getPublishedMessages()).isNotEmpty();
assertThat(kernel.getPublishedMessages()).isNotEmpty();
Message sendMessage = kernel.getPublishedMessages().get(0);
Assertions.assertThat((String) sendMessage.getContent().get(Comm.COMM_ID)).isNotEmpty();
assertThat((String) sendMessage.getContent().get(Comm.COMM_ID)).isNotEmpty();
}

@Test
public void commOpen_sentMessageHasTargetName() throws NoSuchAlgorithmException {
//when
comm.open();
//then
Assertions.assertThat(kernel.getPublishedMessages()).isNotEmpty();
assertThat(kernel.getPublishedMessages()).isNotEmpty();
Message sendMessage = kernel.getPublishedMessages().get(0);
Assertions.assertThat((String) sendMessage.getContent().get(Comm.TARGET_NAME)).isNotEmpty();
assertThat((String) sendMessage.getContent().get(Comm.TARGET_NAME)).isNotEmpty();
}

@Test
Expand All @@ -102,9 +140,9 @@ public void commOpen_sentMessageHasData() throws NoSuchAlgorithmException {
//when
comm.open();
//then
Assertions.assertThat(kernel.getPublishedMessages()).isNotEmpty();
assertThat(kernel.getPublishedMessages()).isNotEmpty();
Message sendMessage = kernel.getPublishedMessages().get(0);
Assertions.assertThat((Map) sendMessage.getContent().get(Comm.DATA)).isNotEmpty();
assertThat((Map) sendMessage.getContent().get(Comm.DATA)).isNotEmpty();
}

@Test
Expand All @@ -114,37 +152,37 @@ public void commOpen_sentMessageHasTargetModule() throws NoSuchAlgorithmExceptio
//when
comm.open();
//then
Assertions.assertThat(kernel.getPublishedMessages()).isNotEmpty();
assertThat(kernel.getPublishedMessages()).isNotEmpty();
Message sendMessage = kernel.getPublishedMessages().get(0);
Assertions.assertThat((String) sendMessage.getContent().get(Comm.TARGET_MODULE)).isNotEmpty();
assertThat((String) sendMessage.getContent().get(Comm.TARGET_MODULE)).isNotEmpty();
}

@Test
public void commClose_shouldSendIOPubSocketMessage() throws NoSuchAlgorithmException {
//when
comm.close();
//then
Assertions.assertThat(kernel.getPublishedMessages()).isNotEmpty();
Assertions.assertThat(kernel.getPublishedMessages().get(0)).isNotNull();
assertThat(kernel.getPublishedMessages()).isNotEmpty();
assertThat(kernel.getPublishedMessages().get(0)).isNotNull();
}

@Test
public void commClose_shouldRemoveCommFromStorageMap() throws NoSuchAlgorithmException {
//when
comm.close();
//then
Assertions.assertThat(kernel.isCommPresent(comm.getCommId())).isFalse();
assertThat(kernel.isCommPresent(comm.getCommId())).isFalse();
}

@Test
public void commClose_sentMessageHasTypeIsCommClose() throws NoSuchAlgorithmException {
//when
comm.close();
//then
Assertions.assertThat(kernel.getPublishedMessages()).isNotEmpty();
assertThat(kernel.getPublishedMessages()).isNotEmpty();
Message sendMessage = kernel.getPublishedMessages().get(0);
Assertions.assertThat(sendMessage.getHeader().getType())
.isEqualTo(JupyterMessages.COMM_CLOSE.getName());
assertThat(sendMessage.getHeader().getType())
.isEqualTo(JupyterMessages.COMM_CLOSE.getName());
}

@Test
Expand All @@ -153,39 +191,39 @@ public void commClose_sentMessageHasEmptyData() throws NoSuchAlgorithmException
//when
comm.close();
//then
Assertions.assertThat(kernel.getPublishedMessages()).isNotEmpty();
assertThat(kernel.getPublishedMessages()).isNotEmpty();
Message sendMessage = kernel.getPublishedMessages().get(0);
Assertions.assertThat((Map) sendMessage.getContent().get(Comm.DATA)).isEmpty();
assertThat((Map) sendMessage.getContent().get(Comm.DATA)).isEmpty();
}

@Test
public void commSend_shouldSendIOPubSocketMessage() throws NoSuchAlgorithmException {
//when
comm.send();
//then
Assertions.assertThat(kernel.getPublishedMessages()).isNotEmpty();
Assertions.assertThat(kernel.getPublishedMessages().get(0)).isNotNull();
assertThat(kernel.getPublishedMessages()).isNotEmpty();
assertThat(kernel.getPublishedMessages().get(0)).isNotNull();
}

@Test
public void commSend_sentMessageHasTypeIsCommClose() throws NoSuchAlgorithmException {
//when
comm.send();
//then
Assertions.assertThat(kernel.getPublishedMessages()).isNotEmpty();
assertThat(kernel.getPublishedMessages()).isNotEmpty();
Message sendMessage = kernel.getPublishedMessages().get(0);
Assertions.assertThat(sendMessage.getHeader().getType())
.isEqualTo(JupyterMessages.COMM_MSG.getName());
assertThat(sendMessage.getHeader().getType())
.isEqualTo(JupyterMessages.COMM_MSG.getName());
}

@Test
public void commSend_sentMessageHasCommId() throws NoSuchAlgorithmException {
//when
comm.send();
//then
Assertions.assertThat(kernel.getPublishedMessages()).isNotEmpty();
assertThat(kernel.getPublishedMessages()).isNotEmpty();
Message sendMessage = kernel.getPublishedMessages().get(0);
Assertions.assertThat((String) sendMessage.getContent().get(Comm.COMM_ID)).isNotEmpty();
assertThat((String) sendMessage.getContent().get(Comm.COMM_ID)).isNotEmpty();
}

@Test
Expand All @@ -194,9 +232,9 @@ public void commClose_sentMessageHasData() throws NoSuchAlgorithmException {
//when
comm.send();
//then
Assertions.assertThat(kernel.getPublishedMessages()).isNotEmpty();
assertThat(kernel.getPublishedMessages()).isNotEmpty();
Message sendMessage = kernel.getPublishedMessages().get(0);
Assertions.assertThat((Map) sendMessage.getContent().get(Comm.DATA)).isNotEmpty();
assertThat((Map) sendMessage.getContent().get(Comm.DATA)).isNotEmpty();
}

private void initCommData(Comm comm) {
Expand Down

0 comments on commit dd34287

Please sign in to comment.