Test uncaught panic produces error message #74

SpencerPark · 2017-09-14T21:33:20Z

This adds a test for ensuring that executing code that panics without recovering returns an execute_reply with an error status as well as publishes an error message over IOPub.

…l during testing

dwhitena

Really great refactor of our testing pattern. Much better than my original. I have commented on some minor things. Also, one of the tests is failing for me:

go test -v
=== RUN   TestEvaluate
--- FAIL: TestEvaluate (1.00s)
        kernel_test.go:305: Should be able to evaluate valid code in notebook cells.
        kernel_test.go:310:   Evaluating code snippet 1/4.
        kernel_test.go:364: content["data"]["test/plain"] "test/plain" field not present
=== RUN   TestPanicGeneratesError
--- PASS: TestPanicGeneratesError (1.00s)
FAIL
exit status 1
FAIL    github.com/gopherdata/gophernotes       2.013s

dwhitena · 2017-09-15T18:33:21Z

kernel_test.go

+const (
+	connectionKey = "a0436f6c-1916-498b-8eb9-e81ab9368e84"
+	sessionID     = "ba65a05c-106a-4799-9a94-7f5631bbe216"
+	transport     = "tcp"


Is the transport likely to change? Maybe we can inline that, as I doubt that we will be running with other types of transport?

dwhitena · 2017-09-15T18:34:00Z

kernel_test.go

+	sessionID     = "ba65a05c-106a-4799-9a94-7f5631bbe216"
+	transport     = "tcp"
+	ip            = "127.0.0.1"
+	shellPort     = 57503


Can we parse the shell and iopub ports from the fixture connection file in the test init? That way, if ports change for any reason we only have to change them in one place.

dwhitena · 2017-09-15T18:34:30Z

kernel_test.go

+	iopubPort     = 40885
+)
+
+type testJupyterClient struct {


dwhitena · 2017-09-15T18:36:54Z

kernel_test.go

+	shellPort     = 57503
+	iopubPort     = 40885
+)
+


To make this file a little more readable I suggest that we shuffle things around a little:

consts and vars //============================================================================== runTest TestMain and any init //============================================================================== Tests //============================================================================== Any other test helps including newTestJupyterClient, sendShellRequest, etc.

dwhitena · 2017-09-15T18:37:53Z

kernel_test.go

+		t.Fatal("iopub.SetSubscribe", err)
+	}
+
+	// wait for a second to give the tcp connection time to complete to avoid missing the early pub messages


dwhitena · 2017-09-15T18:43:07Z

kernel_test.go

+	client.sendShellRequest(t, request)
+	reply = client.recvShellReply(t, timeout)
+
+	// Read the expected 'busy' message and ensure it is in fact, a 'busy' message


dwhitena · 2017-09-15T18:43:49Z

kernel_test.go

+		t.Fatalf("Expected a 'busy' status message but got '%v'", execState)
+	}
+
+	// Read messages from the IOPub channel until an 'idle' message is received


dwhitena · 2017-09-15T18:44:09Z

kernel_test.go

+				t.Fatalf("Expected a 'idle' status message but got '%v'", execState)
+			}
+
+			// Break from the loop as we don't expect any other IOPub messages after the 'idle'


I'll stop nitpicking these. Just FYI.

dwhitena · 2017-09-15T18:45:20Z

kernel_test.go

+func assertMsgTypeEquals(t *testing.T, msg ComposedMsg, expectedType string) {
+	t.Helper()
+
+	if msg.Header.MsgType != expectedType {


I wonder if this should just be inline to make it more readable?

I was thinking the same but as it is already used 3 times and is likely to be used in future tests I figured it was nicer to avoid copy/pasting the message format everywhere. Then to clean up the format like we need to do with the failure char it's just one place that needs changing.

dwhitena · 2017-09-15T18:48:23Z

kernel_test.go

 	}

-	return ""
+	foundPublishedError := false


You can just use var foundPublishedError bool as the zero-value is false.

- Normalize fatal messages to be prefixed with a tab char and error symbol. - Move test helpers to the bottom and better seperate them from the tests. - Read kernel connection info from the test fixture file.

dwhitena

A couple minor nitpicks, then we can merge. I tested locally, and everything seems to work!

go test -v
=== RUN   TestEvaluate
--- PASS: TestEvaluate (4.02s)
        kernel_test.go:96: Should be able to evaluate valid code in notebook cells.
        kernel_test.go:101:   Evaluating code snippet 1/4.
        kernel_test.go:111:     ✓ Should return the correct cell output.
        kernel_test.go:101:   Evaluating code snippet 2/4.
        kernel_test.go:111:     ✓ Should return the correct cell output.
        kernel_test.go:101:   Evaluating code snippet 3/4.
        kernel_test.go:111:     ✓ Should return the correct cell output.
        kernel_test.go:101:   Evaluating code snippet 4/4.
        kernel_test.go:111:     ✓ Should return the correct cell output.
=== RUN   TestPanicGeneratesError
--- PASS: TestPanicGeneratesError (1.00s)
PASS
ok      github.com/gopherdata/gophernotes       5.044s

dwhitena · 2017-09-18T16:00:28Z

kernel_test.go

+	return value
+}
+
+// getString is a test helper that retrieves a value as a map[string]interface{} from the content at the given key.


Name in comment. FYI, running a linter at save time will catch these.

dwhitena · 2017-09-18T16:02:56Z

kernel_test.go

 func TestMain(m *testing.M) {
 	os.Exit(runTest(m))
 }

 // runTest initializes the environment for the tests and allows for
 // the proper exit if the test fails or succeeds.
 func runTest(m *testing.M) int {
+	const connectionFile = "fixtures/connection_file.json"


Maybe move the constants up above?

SpencerPark added 2 commits September 13, 2017 17:20

Create a test jupyter client for capturing interaction with the kerne…

6050317

…l during testing

Test that an uncaught panic produces the proper error messages

01f2b46

dwhitena requested changes Sep 15, 2017

View reviewed changes

SpencerPark mentioned this pull request Sep 15, 2017

Jupyter test client #73

Closed

Reformat kernel_test.

922e21e

- Normalize fatal messages to be prefixed with a tab char and error symbol. - Move test helpers to the bottom and better seperate them from the tests. - Read kernel connection info from the test fixture file.

SpencerPark force-pushed the kernel-tests/panic branch from 60ff976 to 922e21e Compare September 15, 2017 20:05

dwhitena requested changes Sep 18, 2017

View reviewed changes

Fix kernel_test lint issues

aee1cad

dwhitena approved these changes Sep 18, 2017

View reviewed changes

dwhitena merged commit b615ee7 into gopherdata:version-1 Sep 18, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test uncaught panic produces error message #74

Test uncaught panic produces error message #74

SpencerPark commented Sep 14, 2017

dwhitena left a comment

dwhitena Sep 15, 2017

dwhitena Sep 15, 2017

dwhitena Sep 15, 2017

dwhitena Sep 15, 2017

dwhitena Sep 15, 2017

dwhitena Sep 15, 2017

dwhitena Sep 15, 2017

dwhitena Sep 15, 2017

dwhitena Sep 15, 2017

dwhitena Sep 15, 2017

SpencerPark Sep 15, 2017

dwhitena Sep 15, 2017

dwhitena left a comment

dwhitena Sep 18, 2017

dwhitena Sep 18, 2017

Test uncaught panic produces error message #74

Test uncaught panic produces error message #74

Conversation

SpencerPark commented Sep 14, 2017

dwhitena 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

dwhitena left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment