Catch up with CLJS 1.10.520
borkdude opened this issue · 25 comments
There have been several significant improvements and fixes to clojure.spec in CLJS 1.10.516.
Most of them are listed here:
https://github.com/borkdude/speculative/blob/master/doc/issues.md#fixed-and-released
I have some concern here in regards to expound - we are using orchestra instead of vanilla spec here for instrument because expound was not working on 1.10.439.
I can help with QA-ing things 😄
I would really appreciate some help with this, if you guys are up for it. I've spoken a bit with @bhb about the breaking changes in CLJS and ended up reverting some bits so orchestra and expound could continue working together (since they're love & marriage, as far as I'm concerned). Ideally we can move both them forward together.
Another suggestion:
It's possible to support older versions of CLJS via *clojurescript-version* and goog.string/compareVersions. Maybe expound could leverage this.
Example: https://github.com/borkdude/speculative/blob/master/src/speculative/specs.cljc#L9
I was hoping the "Improved Exception Messages and Printing" fixes would fix compatibilities with Expound, but my initial testing indicates that this is only partially fixed. I'll have more time to dig in this weekend.
I have to confirm, kind of taking back what I have said on Slack. I receive now as instrumentation error that does not contain the ex-data when there is a failure and cljs.spec.test.alpha is used.
On the other end, the latest orchestra does its job perfectly on orchestra/instrument and I correctly both data and expound output.
Maybe make a JIRA issue about this in CLJS?
You could submit Orchestra and Expound to https://github.com/cljs-oss/canary so it will be tested automatically and thoroughly.
@arichiardi There are a lot of moving parts here. Can you post a repro including CLJS version, CLJ version, spec version, and if you are calling a function or a macro?
You could submit Orchestra and Expound to https://github.com/cljs-oss/canary so it will be tested automatically and thoroughly.
This is a good idea in general, but wouldn't have caught the particular bug I know about, because the bug is that the REPL doesn't call expound (or any custom printer) correctly. I suppose expound could do some acrobatics to test the REPL layer, but that seems out of scope?
There seem to be tests around CLI usage here: https://github.com/clojure/clojurescript/blob/master/src/test/cljs_cli/cljs_cli/test.clj
So the patch for CLJS-3050 could probably add a test there?
@borkdude That's a good point.
To summarize the situation with 1.10.516 and Expound
- 1.10.516 fixes one the two known issues in 1.10.439
- There is one new minor incompatibility, but it's in a more obscure feature of Expound
- I'll release 0.7.3 soonish to address the minor regression
I'll add some new documentation on Expound/CLJS compat
https://github.com/bhb/expound/blob/078673b8c5bb42f3bcc9a8c2abf53d945d77efdb/doc/compatibility.md
From the compatibility matrix on master I understand that expound works with clj 1.10 + 1.10.516 equally well as it did with 1.0 + 1.10.349?
That's correct.
TL;DR:
- There's a new release of CLJS: 1.10.520.
- Expound works, nothing to fix anymore on that side
- Orchestra can begin catching up
If there's any documentation about what Orchestra changes to vanilla CLJS spec, catching up would be easier: copy the existing code from CLJS and apply those changes, run the tests, done?
Awesome. Thanks, guys, for tracking this down. I'll get the dirty work done with the upstream sync this weekend. We don't have docs for all of the changes, but we do have tests for them.
I've deployed 2019.02.17-SNAPSHOT which should be entirely caught up with CLJS 1.10.520. All Orchestra tests are passing, but I still want to run it through some other code bases and make sure all is good. Feel free to give it a shot and let me know how it goes.
This is also working just fine with the Expound 0.7.2.
@jeaye Thanks!
So far I've seen no errors when running with speculative.
A ret spec violation looks as follows now:
$ clj -A:test:coal-mine -m cljs.main -re node
ClojureScript 1.10.520
cljs.user=> (require '[orchestra-cljs.spec.test :as st])
nil
cljs.user=> (require '[clojure.spec.alpha :as s])
nil
cljs.user=> (defn foo [i] i)
#'cljs.user/foo
cljs.user=> (s/fdef foo :args (s/cat :i string?) :ret number?)
cljs.user/foo
cljs.user=> (st/instrument)
[cljs.user/foo]
cljs.user=> (foo "foo")
Execution error - invalid arguments to cljs.user/foo at (<cljs repl>:1).
"foo" - failed: number?
cljs.user=> *e
#error {:message "Call to #'cljs.user/foo did not conform to spec.", :data #:cljs.spec.alpha{:problems [{:path [], :pred cljs.core/number?, :val "foo", :via [], :in []}], :spec #object[cljs.spec.alpha.t_cljs$spec$alpha3687], :value "foo", :fn cljs.user/foo, :ret "foo", :failure :instrument}}
Is that how it should look?
Everything on Orchestra's end, as far as instrumentation goes, looks correct there. However, due to the fact that the REPL is now the one printing the spec failure, you're not getting proper reporting of the return value and spec. Will you try the same thing, but with Expound enabled? I believe that should handle the :ret failures properly.
It could be that Orchestra should either require Expound or that we need to implement our own printing of spec failures in the REPL. Basically the same code as upstream, but with :ret and :fn support.
I was just checking. It seems to be it would be nice to be able to plug your own error printers and not require a specific one (expound) although that could be recommended in the README?
or that we need to implement our own printing of spec failures in the REPL
Maybe as a feature in a future version?
Maybe as a feature in a future version?
It may be necessary, even for this version. Otherwise, REPL reporting is borked. I'm not familiar with how to hook up custom REPL error printers. If any of you feel like taking a crack at it, by all means, please do. I'll be able to take a look at it over this weekend otherwise. I'll leave this build as a snapshot until this is sorted out.
I don't get it... with [orchestra "2018.12.06-2"] here's the message:
Call to xanadu.pages.product-selection.common/form-valid? did not conform to spec:↵
<filename missing>:<line number missing>↵
↵
-- Spec failed --------------------↵
↵
Return value↵
↵
true↵
↵
should satisfy↵
↵
int?↵
↵
-------------------------↵
Detected 1 error↵
it doesn't pick up line number which is not very helpful, but not a big deal.
So I replaced it with [orchestra "2019.02.17-SNAPSHOT"] and now the message simply says:
Call to #'xanadu.pages.product-selection.common/form-valid? did not conform to spec.
So it now become even less helpful?
So it now become even less helpful?
Correct, since error reporting is no longer done by spec, it's done by the REPL printer. That is precisely what I meant when I said (in the comment above yours):
It may be necessary, even for this version. Otherwise, REPL reporting is borked.
Furthermore, if you have something catching spec errors outside the REPL, you'll now need to manually print them using spec's explain functions or something link expound. This has nothing to do with Orchestra and is just the way upstream spec is dealing with things now. It's more flexible, but it's more work for everyone.