Recent failures in tools.nrepl

[Alex Miller]

After the recent hashing changes in 1.6, the tools.nrepl build started failing. Any chance someone could take a look? I'd suspect somewhere there is something hash-order dependent. This is different than the spurious exceptions we see sometimes in tools.nrepl.

Example:

http://build.clojure.org/job/tools.nrepl-test-matrix/125/CLOJURE_VERSION=1.6.0-master-SNAPSHOT,jdk=IBM%20JDK%201.6/console

Testing clojure.tools.nrepl.middleware-test

FAIL in (append-dependency-free-middleware) (middleware_test.clj:69)
expected: (<= (stack n) (stack m) (count stack))
  actual: (not (<= 7 6 8))

[Colin Jones]

I've isolated it to a difference between the way 1.5 and 1.6 sort a
particular PersistentHashSet when there's a tie. It's a big one, so
here's a small example with the same problem:

;; on 1.5
user=> (sort-by :foo #{{:foo 1 :bar 2} {:foo 1 :bar 3}})
;=> ({:foo 1, :bar 3} {:foo 1, :bar 2})

;; on 1.6
user=> (sort-by :foo #{{:foo 1 :bar 2} {:foo 1 :bar 3}})
;=> ({:bar 2, :foo 1} {:bar 3, :foo 1})

I assume this is because sort-by depends (a few levels down the stack)
on Arrays.sort, which is guaranteed to be stable - and so I'm guessing
the seq() order of PersistentHashSet changed.

That said, I can't see any reason why this test
(https://github.com/clojure/tools.nrepl/blob/ddadae84cb470770fb13b3540fcbdda7944bd00b/src/test/clojure/clojure/tools/nrepl/middleware_test.clj#L64-L69)
should need care about the difference in sort order between `n` and
`m`, since they tie at 0 for the number of elements:
https://github.com/clojure/tools.nrepl/blob/ddadae84cb470770fb13b3540fcbdda7944bd00b/src/main/clojure/clojure/tools/nrepl/middleware.clj#L125

So maybe the test ought to just change to be 2 assertions?

(is (<= (count default-middlewares) (stack m) (count stack)))
(is (<= (count default-middlewares) (stack n) (count stack)))

... or something like that?

But there's also comment here
(https://github.com/clojure/tools.nrepl/blob/ddadae84cb470770fb13b3540fcbdda7944bd00b/src/main/clojure/clojure/tools/nrepl/middleware.clj#L116)
that makes me think there's *supposed* to be some special sorting for
descriptor-less middleware and it's just not working.

That is to say, maybe Chas knows whether this a real bug or a test bug? :)

- Colin

> --
> You received this message because you are subscribed to the Google Groups
> "clojure-tools" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to clojure-tool...@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.



--
Colin Jones
@trptcolin

[Chas Emerick]

Meta process note: it'd be great if the Hudson configs could be modified
to email a notification of failed builds to the respective project
leads. I'd have been on this before now, and the transient matrix
failure would have stayed on my radar. I see the option for it in the
'Configure' screen, but I know Stuart rebuilds those configurations from
scratch via some automated process, so I presume changing anything there
will be reverted eventually.

Anyway: the test was a bit sloppy. The only guarantee is that
middlewares without explicit dependencies will land at the end of the
linearization. At some point, it'd be nice if their provided order was
maintained, but that's wasn't something I wanted to bother offering when
the linearization was implemented.

Colin is right about what fundamentally caused the change (set-member
sorting), and that there are some dodgy bits in the implementation;
c'est la vie. The sort-by/reverse thing prior to the conj-sorted
reduction is particularly pointless, but I'm disinclined to touch
anything in there unless I'm going to set down for a time and do it
properly, etc.

I've pushed a clarification of the test to more properly check the
invariant that is actually guaranteed; the nREPL hudson build is now green.

(P.S. The one failure in the matrix build I saw this morning was this:
http://build.clojure.org/job/tools.nrepl-test-matrix/CLOJURE_VERSION=1.2.0,jdk=IBM%20JDK%201.6/129/consoleFull
I've never seen that before.)

- Chas

[Alex Miller]

I have an item on my todo list already to update the config template with build notification, just haven't gotten to it yet. The only hard part is knowing what magic xml to put in the hudson template. :)  

Thanks for taking a look. 

That test failure is due to a bug in Clojure 1.2.0 keywords that was fixed in Clojure 1.2.1.

email to clojure-tools+unsubscribe@googlegroups.com.

For more options, visit https://groups.google.com/groups/opt_out.

--

You received this message because you are subscribed to a topic in the Google Groups "clojure-tools" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/clojure-tools/RhgUiYiF8NY/unsubscribe.
To unsubscribe from this group and all its topics, send an email to clojure-tools+unsubscribe@googlegroups.com.