Actually fix #537 #539
Actually fix #537 #539
Conversation
- revert `cider.nrepl/init` such that it takes a vector of middlewares - have `cider.nrepl/init` pass a map of arguments to `start-nrepl` - tweak cider nrepl boot task to pass a map to `start-nrepl` directly - set defaults for `port` and `bind` to keep `nrepl-server` happy
It appears travis hosts don't have IPv6 enabled, and `nrepl.server/start-server`s claim to fall back to localhost if `::` can't be resolved is not entirely true ...
| [clojure.tools.nrepl.server :as nrepl.server] | ||
| [clojure.tools.nrepl.transport :as transport])) | ||
|
|
||
| (defn start-stop-nrepl-session [opts] |
There was a problem hiding this comment.
I don't see where you stop the server in each test.
There was a problem hiding this comment.
My understanding was that the with-open macro would suffice to close the server. I borrowed much of this from cider.nrepl.middleware.debug-integration-test/with-nrepl-session*
| [middleware-var-strs] | ||
| (into [] mw-xf middleware-var-strs)) | ||
|
|
||
| (defn- build-handler |
There was a problem hiding this comment.
I think that build handler should just take a list of middlewares and be invoked only if a handler wasn't passed to start-nrepl.
There was a problem hiding this comment.
I see, so that implies that either a handler or middleware is expected, but not both. I can make that change.
There was a problem hiding this comment.
Yeah, exactly. Normally you'd just use a predefined handler, but in some cases you might opt to build the handler yourself by supplying some list of middlewares instead.
There was a problem hiding this comment.
(e.g. because some middleware is problematic in your setup or something like this)
src/cider_nrepl/main.clj
Outdated
| list of middleware." | ||
| [{:keys [bind port] :as opts}] | ||
| (let [handler | ||
| (build-handler opts) |
There was a problem hiding this comment.
Here I'd (or handler (build-handler). (provided it's extracted from the ops map above.
| (nrepl.server/start-server :handler handler | ||
| :bind bind | ||
| :port port) | ||
| :bind (or bind "localhost") |
There was a problem hiding this comment.
So, I think what will happen here is an exception when this goes through travis. From clojure.tools.nrepl.server:
(let [bind-addr (if bind
(InetSocketAddress. ^String bind ^Integer port)
(let [local (InetSocketAddress. "::" port)]
(if (.isUnresolved local)
(InetSocketAddress. "localhost" port)
local)))
nil causes (InetSocketAddress. "::" port) to be invoked, which throws an exception in travis - I presume due to IPv6 not being enabled. If we default to localhost, we dodge that particular idiosyncrasy.
There was a problem hiding this comment.
Sorry I should clarify: I made this change after experiencing the exception in travis. Not a thought experiment, but an observation :)
There was a problem hiding this comment.
I see. I was just wondering if this won't message up something for people who have disabled IPv4. Probably not a big deal now, I'll fixed this once we finally move the project to https://github.com/nrepl/nREPL
There was a problem hiding this comment.
(tools.nrepl is practically dead, so we've migrated the project out of clojure contrib to revive it)
Codecov Report
@@ Coverage Diff @@
## master #539 +/- ##
==========================================
+ Coverage 72.6% 73.34% +0.74%
==========================================
Files 36 36
Lines 2303 2307 +4
Branches 134 136 +2
==========================================
+ Hits 1672 1692 +20
+ Misses 497 479 -18
- Partials 134 136 +2
Continue to review full report at Codecov.
|
My fix yesterday was not properly tested - I started nREPL from the command line, but I didn't attempt to exercise it with messages. As a result, I missed the fact I was setting passing the middleware as the handler, which kept things just as broken.
This PR contains a fix for that problem, plus a new test ns
cider.nrepl.main-testthat exercises thestart-nreplfunction by passing it a variety of valid and invalid option maps. The tests go as far as checking that two:op "clone"messages can be exchanged before declaring success.On my local machine I saw a test failure in a totally unrelated part of the code,
test-sum-types-is-base64, which I'm assuming is an artifact of my local environment and not something that will come up in the PR tests.