Check what clients need from load-file

CHANGELOG.md

@@ -30,3 +30,5 @@ Initial release.
   CI of a server.
 - [#11](https://github.com/nrepl/proof/pull/11): Checks for `interrupt`, and
   scenarios for nREPL's interrupt bugs in `proof serve`.
+- [#12](https://github.com/nrepl/proof/pull/12): Checks for `load-file`, and
+  `load-file` in `proof serve`.

PLAN.md

@@ -92,9 +92,12 @@ lookups and completions, plus requests that go wrong.
 
 ### load-file
 
-- Checks for `load-file` with the fields CIDER, Calva and Conjure send.
-  The spec and the servers don't agree on which field has the contents
-  of the file ([spec#3](https://github.com/nrepl/spec.nrepl.org/issues/3)).
+- Done: checks for the value of `load-file` and for a load that fails,
+  where CIDER needs `eval-error`, Calva `ex` and Conjure `err`, plus what
+  CIDER, Calva and Conjure send to load a file. The spec and the servers
+  don't agree on which field has the contents of the file
+  ([spec#3](https://github.com/nrepl/spec.nrepl.org/issues/3)), but every
+  server in the matrix takes `file` and passes them all.
 
 ### Lookups and Completions
 

README.md

@@ -131,14 +131,14 @@ the replies). `-like jank` turns on all of jank's scenarios at once, and
 ## Status
 
 proof is still in its early days. Right now it covers the core of the
-protocol (`describe`, unknown ops, `eval`, sessions, `stdin`, `interrupt` and the wire format),
+protocol (`describe`, unknown ops, `eval`, sessions, `stdin`, `interrupt`, `load-file` and the wire format),
 what CIDER, Calva, Conjure, vim-fireplace, REPLy and neorepl send while
-connecting and evaluating code, the requests
+connecting, evaluating code and loading files, the requests
 clients send and the server differences clients have to deal with, and
 `proof list` will show you all the checks.
 
-Checks for `load-file`, lookups, completions and malformed requests are
-coming next, along with the first release.
+Checks for lookups, completions and malformed requests are coming next,
+along with the first release.
 [PLAN.md](PLAN.md) has all the details.
 
 ## Why "proof"?

doc/design.md

@@ -306,6 +306,6 @@ to have a single test suite that every server can be checked against.
 ## Future Plans
 
 At this point proof covers the core of the protocol (`describe`, unknown
-ops, sessions, `eval`, `stdin`, `interrupt` and the wire format), the requests of
+ops, sessions, `eval`, `stdin`, `interrupt`, `load-file` and the wire format), the requests of
 clients and the server differences clients have to deal with.
 [PLAN.md](../PLAN.md) has what's planned next.

doc/hacking.md

@@ -256,7 +256,7 @@ besides `done`:
 | Option | What the reply needs |
 |---|---|
 | `new-session` | A `new-session`, which later steps can use as `$` and this name (e.g. `session = "$repl"`). |
-| `snippet` | The value of this snippet of the server's profile, with all the parts it came in joined together. Its code goes in the request, as it stands for the user's code. |
+| `snippet` | The value of this snippet of the server's profile, with all the parts it came in joined together. Its code goes in the request (in `file` for `load-file`), as it stands for the user's code. |
 | `values` | At least this many values, told apart by the `ns` that comes with each one (or after it), which is how some clients tell them apart. |
 | `dicts` | These fields have to be dicts (or empty lists), if the reply has them. Each one is a list of keys, e.g. `["versions", "clojure"]`. |
 

doc/profiles.md

@@ -130,10 +130,10 @@ snippet is missing, all the checks that need it are skipped.
 
 | Snippet | Options | What it should do | Checks that use it |
 |---|---|---|---|
-| `value` | `code`, `value` | evaluate to `value` | `eval.value`, `eval.survives-error`, `eval.ns`, `eval.unknown-ns`, `session.ephemeral`, `session.unknown`, `session.closed`, `cider.eval`, `cider.repl`, `calva.eval`, `conjure.eval`, `fireplace.eval`, `neorepl.eval` |
+| `value` | `code`, `value` | evaluate to `value` | `eval.value`, `eval.survives-error`, `eval.ns`, `eval.unknown-ns`, `session.ephemeral`, `session.unknown`, `session.closed`, `load-file.value`, `cider.eval`, `cider.repl`, `cider.load-file`, `calva.eval`, `calva.load-file`, `conjure.eval`, `conjure.load-file`, `fireplace.eval`, `neorepl.eval` |
 | `stdout` | `code`, `out` | print `out` to the standard output | `eval.stdout`, `eval.stdout-order` |
 | `stderr` | `code`, `err` | print `err` to the standard error | `eval.stderr` |
-| `throw` | `code` | raise an error | `eval.error-status`, `eval.error-report`, `eval.survives-error` |
+| `throw` | `code` | raise an error | `eval.error-status`, `eval.error-report`, `eval.survives-error`, `load-file.error` |
 | `define` | `code`, `use`, `value` | define something that `use` reads back as `value` | `session.persistent` |
 | `session-state` | `code`, `use`, `value` | leave some state in the session that `use` reads back as `value` | `session.across-connections`, `session.isolated` |
 | `multiple` | `code`, `values` | consist of several forms that evaluate to `values` (in order) | `eval.multiple-forms` |

doc/spec-changes.md

@@ -50,6 +50,11 @@ uses `file-path`, but all the servers we've looked at expect `file` (the
 contents of the file), and some of them accept `file-path` and
 `file-name` as well.
 
+The spec doesn't say what a failed load looks like either, and clients
+tell it by different things: CIDER by `eval-error`, Calva only by `ex`
+(it reports a load without `ex` as a success) and Conjure only shows
+`err`. A failed load should have all three, just like a failed eval.
+
 ### Where `interrupted` Goes
 
 The spec's example reuses the `id` of the eval request for the interrupt

doc/usage.md

@@ -100,12 +100,14 @@ eval
 ```
 
 The checks in the `cider`, `calva`, `conjure`, `fireplace`, `reply` and
-`neorepl` groups send what those clients send while connecting and
-evaluating code, with all the extra fields they put in their requests.
+`neorepl` groups send what those clients send while connecting,
+evaluating code and loading files, with all the extra fields they put in
+their requests.
 If one of them fails, the client won't work properly with your server,
 and the report says what the client does with the reply it got (e.g.
 "CIDER gives up connecting"). The code they evaluate for the user is the
-`value` snippet of your profile. (REPLy sends nothing but the code and
+`value` snippet of your profile, which goes in a file for `load-file`.
+(REPLy sends nothing but the code and
 the session, so `eval.value` covers it.) Before evaluating code from a
 source file, CIDER, Calva and Conjure evaluate an `ns` form for the
 file's namespace, so those checks also need the `clojure` capability.
@@ -424,7 +426,9 @@ Other functions get the error Clojure gives for a symbol it can't
 resolve, and syntax proof doesn't read (e.g. sets or anonymous functions)
 gets a read error. That's enough for CIDER, Calva, Conjure and
 vim-fireplace to connect and work, and it's all you need for checking
-output, values, errors, input and interrupts.
+output, values, errors, input and interrupts. `load-file` evaluates
+what's in the file the same way, and like nREPL it sends only the value
+of the last form and stops at the first error.
 
 When you stop it, `proof serve` checks the requests your client sent,
 just like `proof proxy` does, with the same report, exit codes and

internal/checks/all.go

@@ -11,6 +11,7 @@ func All() []*check.Check {
 	all = append(all, evalChecks()...)
 	all = append(all, stdinChecks()...)
 	all = append(all, interruptChecks()...)
+	all = append(all, loadFileChecks()...)
 	all = append(all, clientProfileChecks()...)
 	return all
 }

internal/checks/checks_test.go

@@ -57,17 +57,23 @@ func TestChecksCatchMisbehaviour(t *testing.T) {
 	for _, c := range interruptChecks() {
 		noInterrupts[c.ID] = S
 	}
+	noLoadFile := map[string]check.Verdict{}
+	for _, c := range loadFileChecks() {
+		noLoadFile[c.ID] = S
+	}
+	// What CIDER, Calva and Conjure send to load a file.
+	clientsLoad := map[string]check.Verdict{"cider.load-file": F, "calva.load-file": F, "conjure.load-file": F}
 	cases := []struct {
 		name string
 		q    quirks
 		want map[string]check.Verdict
 	}{
 		// Conjure does without the features that need ops.
-		{"ops as a list", quirks{opsList: true}, checktest.Merged(connects, noInterrupts, map[string]check.Verdict{"describe.ops-dict": F,
+		{"ops as a list", quirks{opsList: true}, checktest.Merged(connects, noInterrupts, noLoadFile, map[string]check.Verdict{"describe.ops-dict": F,
 			"describe.required-ops": S, "stdin.need-input": S, "stdin.roundtrip": S, "stdin.eof": S, "conjure.connect": check.Pass})},
 		{"clone not advertised", quirks{noClone: true}, map[string]check.Verdict{"describe.required-ops": F}},
 		{"no versions", quirks{noVersions: true}, map[string]check.Verdict{"describe.versions": W}},
-		{"describe kills the connection", quirks{crashOnDescribe: true}, checktest.Merged(connects, noInterrupts, map[string]check.Verdict{
+		{"describe kills the connection", quirks{crashOnDescribe: true}, checktest.Merged(connects, noInterrupts, noLoadFile, map[string]check.Verdict{
 			"describe.reply": F, "describe.ops-dict": S, "describe.required-ops": S, "describe.versions": S,
 			"stdin.need-input": S, "stdin.roundtrip": S, "stdin.eof": S})},
 		{"no unknown-op", quirks{noUnknownOp: true}, map[string]check.Verdict{"op.unknown": F, "op.unknown-echo": W}},
@@ -77,9 +83,9 @@ func TestChecksCatchMisbehaviour(t *testing.T) {
 			"op.unknown": F, "op.unknown-echo": F, "wire.status-type": F, "conjure.connect": F}},
 		{"no session-closed", quirks{noSessionClosed: true}, map[string]check.Verdict{"session.close": F}},
 		// Only clients put dicts and lists in their requests.
-		{"flat fields only", quirks{flatFields: true}, map[string]check.Verdict{
+		{"flat fields only", quirks{flatFields: true}, checktest.Merged(clientsLoad, map[string]check.Verdict{
 			"cider.connect": F, "cider.eval": F, "cider.repl": F, "calva.connect": F, "calva.eval": F, "conjure.eval": F,
-			"neorepl.eval": F}},
+			"neorepl.eval": F})},
 		{"any session accepted", quirks{acceptAnySession: true}, map[string]check.Verdict{"session.unknown": F, "session.closed": F}},
 		{"shared session state", quirks{sharedState: true}, map[string]check.Verdict{"session.isolated": F}},
 		{"sessions tied to sockets", quirks{socketSessions: true}, map[string]check.Verdict{"session.across-connections": W,
@@ -88,8 +94,9 @@ func TestChecksCatchMisbehaviour(t *testing.T) {
 		{"last value only", quirks{lastValueOnly: true}, map[string]check.Verdict{"eval.multiple-forms": F, "fireplace.connect": F}},
 		{"stderr dropped", quirks{dropErr: true}, map[string]check.Verdict{"eval.stderr": F}},
 		{"output after value", quirks{outAfterValue: true}, map[string]check.Verdict{"eval.stdout-order": W}},
-		{"no eval-error", quirks{noEvalError: true}, map[string]check.Verdict{"eval.error-status": F}},
-		{"no ex", quirks{noEx: true}, map[string]check.Verdict{"eval.error-report": W}},
+		{"no eval-error", quirks{noEvalError: true}, map[string]check.Verdict{"eval.error-status": F, "load-file.error": F}},
+		// Calva tells a failed load by its ex alone.
+		{"no ex", quirks{noEx: true}, map[string]check.Verdict{"eval.error-report": W, "load-file.error": F}},
 		{"no no-code", quirks{noNoCode: true}, map[string]check.Verdict{"eval.no-code": W}},
 		// REPLy evaluates nothing before its first prompt.
 		{"empty code never answered", quirks{emptyCodeHangs: true}, map[string]check.Verdict{"reply.connect": F}},
@@ -105,11 +112,11 @@ func TestChecksCatchMisbehaviour(t *testing.T) {
 		{"state after eval-error", quirks{errorThenState: true}, map[string]check.Verdict{}},
 		{"session not echoed", quirks{noSessionEcho: true}, map[string]check.Verdict{"wire.session-echo": F}},
 		{"output without id", quirks{noID: true}, map[string]check.Verdict{
-			"wire.id": W, "eval.stdout": F, "eval.stderr": F, "eval.error-report": W}},
-		{"integer value", quirks{intValue: true}, map[string]check.Verdict{
+			"wire.id": W, "eval.stdout": F, "eval.stderr": F, "eval.error-report": W, "load-file.error": F}},
+		{"integer value", quirks{intValue: true}, checktest.Merged(clientsLoad, map[string]check.Verdict{
 			"wire.field-types": F, "eval.value": F, "eval.survives-error": F, "eval.multiple-forms": F,
 			"session.ephemeral": F, "session.persistent": F, "cider.eval": F, "cider.repl": F, "calva.eval": F, "conjure.eval": F, "fireplace.eval": F,
-			"neorepl.eval": F, "interrupt.survives": F, "interrupt.queued": F}},
+			"neorepl.eval": F, "interrupt.survives": F, "interrupt.queued": F, "load-file.value": F})},
 		{"unsorted keys", quirks{unsortedKeys: true}, map[string]check.Verdict{"wire.canonical": W}},
 		{"invalid UTF-8", quirks{badUTF8: true}, map[string]check.Verdict{"wire.utf8": W, "eval.stdout": F}},
 		{"no stdin op", quirks{noStdinOp: true}, map[string]check.Verdict{
@@ -133,6 +140,11 @@ func TestChecksCatchMisbehaviour(t *testing.T) {
 		{"interrupted on the wrong connection", quirks{interruptHere: true}, map[string]check.Verdict{
 			"interrupt.other-connection": F, "wire.id": W}},
 		{"session dies on interrupt", quirks{diesOnInterrupt: true}, map[string]check.Verdict{"interrupt.survives": F, "interrupt.queued": F}},
+		// CIDER and Conjure send load-file without asking, and so does
+		// Calva's profile, as it can't load files either way.
+		{"no load-file op", quirks{noLoadFileOp: true}, checktest.Merged(noLoadFile, clientsLoad)},
+		{"load-file without a value", quirks{loadFileNoValue: true}, checktest.Merged(clientsLoad,
+			map[string]check.Verdict{"load-file.value": F})},
 	}
 	for _, c := range cases {
 		t.Run(c.name, func(t *testing.T) {

internal/checks/client_profiles.go

@@ -46,8 +46,8 @@ type clientStep struct {
 	// an earlier step got.
 	Send request `toml:"send"`
 	// Snippet names a snippet of the server's profile whose code goes in
-	// the request, and whose value the reply has to have. It stands for
-	// the user's code.
+	// the request (in file for load-file, and code otherwise), and whose
+	// value the reply has to have. It stands for the user's code.
 	Snippet string `toml:"snippet"`
 	// NewSession names the session the reply has to hand back.
 	NewSession string `toml:"new-session"`
@@ -147,8 +147,13 @@ func (c clientCheck) replay(t *check.T) {
 		}
 		var want string
 		if s.Snippet != "" {
+			// The user's code is in a file for load-file.
+			field := "code"
+			if req.Str("op") == "load-file" {
+				field = "file"
+			}
 			sn := t.Snippet(s.Snippet)
-			req["code"], want = sn.Code, sn.Value
+			req[field], want = sn.Code, sn.Value
 		}
 		step := fmt.Sprintf("step %d (%s)", i+1, req.Str("op"))
 		resp := t.Request(conn, req)

internal/checks/client_profiles_test.go

@@ -21,8 +21,7 @@ func TestClientProfiles(t *testing.T) {
 	}
 	pinned := regexp.MustCompile(`^https://github\.com/[^/]+/[^/]+/blob/[0-9a-f]{40}/`)
 	// The links in refs.go to the same clients.
-	bases := map[string]string{"CIDER": ciderBase, "Calva": strings.TrimSuffix(calvaBase, "nrepl/index.ts"),
-		"Conjure": strings.TrimSuffix(conjureBase, "client/clojure/nrepl/server.fnl"), "vim-fireplace": fireplaceBase,
+	bases := map[string]string{"CIDER": ciderBase, "Calva": calvaSrc, "Conjure": conjureSrc, "vim-fireplace": fireplaceBase,
 		"REPLy": strings.TrimSuffix(replyBase, "eval_modes/nrepl.clj")}
 	for _, p := range clientProfiles() {
 		if p.Name == "" || !pinned.MatchString(p.Code) {

internal/checks/clients/calva.toml

@@ -79,3 +79,30 @@ column = 1
 pprint = 1
 "nrepl.middleware.print/print" = "cider.nrepl.pprint/pprint"
 "nrepl.middleware.print/options" = { right-margin = 120, length = 50 }
+
+[[checks]]
+id = "calva.load-file"
+title = "Calva can load a file"
+why = "Loading a file sends what's in it with load-file, along with the file's path, copies of its name and path in fields of Calva's own and options for printing the value, so a server that can't handle them breaks loading files in Calva."
+
+[[checks.steps]]
+send = { op = "clone", client-name = "Calva", client-version = "2.0.599" }
+new-session = "calva"
+why = "Calva gives up connecting after 30 seconds"
+refs = ["nrepl/index.ts#L277-L286", "nrepl/index.ts#L243-L253"]
+
+[[checks.steps]]
+snippet = "value"
+why = "Calva says there were no results from the file"
+refs = ["nrepl/index.ts#L783-L817", "evaluate.ts#L786-L793"]
+
+[checks.steps.send]
+op = "load-file"
+session = "$calva"
+file-name = "calva.clj"
+file-path = "/home/user/project/src/proof/calva.clj"
+fileName = "calva.clj"
+filePath = "/home/user/project/src/proof/calva.clj"
+pprint = 1
+"nrepl.middleware.print/print" = "cider.nrepl.pprint/pprint"
+"nrepl.middleware.print/options" = { right-margin = 120, length = 50 }

internal/checks/clients/cider.toml

@@ -116,3 +116,28 @@ column = 7
 "nrepl.middleware.print/buffer-size" = 4096
 "nrepl.middleware.print/options" = { right-margin = 70 }
 content-type = "true"
+
+[[checks]]
+id = "cider.load-file"
+title = "CIDER can load a buffer"
+why = "Loading a buffer (C-c C-k) sends what's in it with load-file, along with the buffer's path and options for printing the value, so a server that can't handle them breaks loading buffers in CIDER."
+
+[[checks.steps]]
+send = { op = "clone", client-name = "CIDER", client-version = "2.1.0-snapshot" }
+new-session = "repl"
+why = "CIDER gives up connecting"
+refs = ["nrepl-client.el#L749-L760"]
+
+[[checks.steps]]
+snippet = "value"
+why = "CIDER doesn't show the result and never marks the buffer as loaded"
+refs = ["cider-client.el#L799-L817", "cider-client.el#L572-L586", "cider-eval.el#L614-L635"]
+
+[checks.steps.send]
+op = "load-file"
+session = "$repl"
+file-path = "/home/user/project/src/proof/cider.clj"
+file-name = "cider.clj"
+"nrepl.middleware.print/print" = "cider.nrepl.pprint/pr"
+"nrepl.middleware.print/stream?" = []
+"nrepl.middleware.print/quota" = 1048576

internal/checks/clients/conjure.toml

@@ -149,3 +149,27 @@ line = 3
 column = 1
 "nrepl.middleware.print/print" = "cider.nrepl.pprint/pprint"
 "nrepl.middleware.print/options" = { associative = 1, length = 500, level = 50, right-margin = 72 }
+
+[[checks]]
+id = "conjure.load-file"
+title = "Conjure can evaluate a file"
+why = "Evaluating a file sends what's in it with load-file, along with the file's path and options for printing the value, so a server that can't handle them breaks evaluating files in Conjure."
+
+[[checks.steps]]
+send = { op = "clone", client-name = "Conjure" }
+new-session = "conjure"
+why = "Conjure gives up waiting after 10 seconds and evaluates code without a session, so nothing (e.g. *1 or *e) carries over from one evaluation to the next"
+refs = ["client/clojure/nrepl/server.fnl#L259-L272", "client/clojure/nrepl/server.fnl#L379-L392", "remote/nrepl.fnl#L48-L57"]
+
+[[checks.steps]]
+snippet = "value"
+why = "users don't see the result of the file"
+refs = ["client/clojure/nrepl/server.fnl#L154-L165", "client/clojure/nrepl/ui.fnl#L24-L50"]
+
+[checks.steps.send]
+op = "load-file"
+session = "$conjure"
+file-name = "conjure.clj"
+file-path = "/home/user/project/src/proof/conjure.clj"
+"nrepl.middleware.print/print" = "cider.nrepl.pprint/pprint"
+"nrepl.middleware.print/options" = { associative = 1, length = 500, level = 50, right-margin = 72 }