Cljs tests failing under new version of planck/cljs

[lread]

Hi @kkinnear, as you know I like to test current releases of libraries that use rewrite-clj against the rewrite-clj development branch. When upgrading my tests to use zprint 1.2.2 I experienced a failure when running the zprint ClojureScript tests.

And I can reproduce the failure with zprint proper.

My environment

• macOS 10.15.7
• JVM Temurin-11.0.14+9
• Planck 2.26.0
• Clojure 1.10.3.1075

Steps

git clone https://github.com/kkinnear/zprint.git
cd zprint
git checkout 1.2.2
clojure -M:cljs-runner

Result

Testing zprint.config-test

Testing zprint.core-test

Testing zprint.finish-test

Testing zprint.guide-test

Testing zprint.main-test

Testing zprint.range-test

Testing zprint.rewrite-test

Testing zprint.spec-test

Testing zprint.zprint-test

FAIL in (zprint-tests) (file:44:173)

(try (zprint-str "[{k 1 g 2 c 3 aaa}]" {:parse-string? true, :style :odr}) (catch :default e (str e)))

matches: ""
>>>  expected diverges: "[{aaa, c 3, g 2, k 1}]"
>>>    actual diverges: "Error: Unable to parse the string '[{k 1 g 2 c 3 aaa}]' because of 'Error: No value supplied for key: aaa'.  Consider adding any unallowed elements to {:parse {:ignore-if-parse-fails #{ <string> }}}"

expected: "[{aaa, c 3, g 2, k 1}]"
  actual: "Error: Unable to parse the string '[{k 1 g 2 c 3 aaa}]' because of 'Error: No value supplied for key: aaa'.  Consider adding any unallowed elements to {:parse {:ignore-if-parse-fails #{ <string> }}}"

    diff: - "[{aaa, c 3, g 2, k 1}]"
          + "Error: Unable to parse the string '[{k 1 g 2 c 3 aaa}]' because of 'Error: No value supplied for key: aaa'.  Consider adding any unallowed elements to {:parse {:ignore-if-parse-fails #{ <string> }}}"


FAIL in (zprint-tests) (file:44:173)

(try (zprint-str vbm {:parse-string? true, :remove {:map {:key-no-sort #{"..."}}, :parse {:ignore-if-parse-fails #{"..."}}}, :style :odr}) (catch :default e (str e)))

matches: ""
>>>  expected diverges: "[{..., :a 1, :b 2}]"
>>>    actual diverges: "Error: Unable to parse the string '[{:a 1 :b 2 ...}]' because of 'Error: No value supplied for key: ...'.  Consider adding any unallowed elements to {:parse {:ignore-if-parse-fails #{ <string> }}}"

expected: "[{..., :a 1, :b 2}]"
  actual: "Error: Unable to parse the string '[{:a 1 :b 2 ...}]' because of 'Error: No value supplied for key: ...'.  Consider adding any unallowed elements to {:parse {:ignore-if-parse-fails #{ <string> }}}"

    diff: - "[{..., :a 1, :b 2}]"
          + "Error: Unable to parse the string '[{:a 1 :b 2 ...}]' because of 'Error: No value supplied for key: ...'.  Consider adding any unallowed elements to {:parse {:ignore-if-parse-fails #{ <string> }}}"


Testing zprint.zutil-test

Ran 9 tests containing 1397 assertions.
2 failures, 0 errors.

More tests

If I downgrade to sprint v1.2.1 and rerun:

git checkout 1.2.1
clojure -M:cljs-runner

I get the same failure. Tests for v1.2.1 used to pass for me, so the failure seems to be triggered by the new version of planck.

Other Notes

• Clojure tests passed.
• FWIW, I also run rewrite-clj tests under the current version of planck and they seem to be fine.

[lread]

After downgrading to planck 2.25.0 (which is not a mountain of fun to do via brew), I can confirm that cljs tests pass for me for zprint v1.2.1 and v1.2.2.

There might be a better way to downgrade planck via brew, but here's what I managed to figure out.

1. Create a new tap, any name will do, I used:

   brew tap-new lee/homebrew-old
   

2. Extract v2.25.0 of planck to this new tap

   brew extract --version 2.25.0 planck lee/homebrew-old
   

3. Install v2.25.0, this took 40 minutes on my system, so it must be building form source:

   brew install lee/homebrew-old/planck@2.25.0
   

4. Overwrite currently installed planck with v2.25.0 via:

   brew link --overwrite planck@2.25.0
   

[kkinnear]

Hey @lread, it is good to hear from you. Not that I like what I'm hearing, mind you.

Thanks for figuring out that the change was related to the planck change. I am indeed running

% planck --version
2.25.0 

And mine is also a version for arm/Apple Silicon, as the Intel version runs terribly with the Rosetta thing on the newer Macs.

In looking at the error, I'm rather confused. This is the test:

  #?(:clj
       (expect
         "java.lang.Exception: Unable to parse the string '[{k 1 g 2 c 3 aaa}]'
because of 'java.lang.IllegalArgumentException: No value supplied for key: aaa'.
  Consider adding any unallowed elements to {:parse {:ignore-if-parse-fails #{ <
string> }}}"
         (try (zprint-str "[{k 1 g 2 c 3 aaa}]"
                          {:parse-string? true, :style :odr})
              (catch Exception e (str e))))
     :cljs (expect "[{aaa, c 3, g 2, k 1}]"
                   (try (zprint-str "[{k 1 g 2 c 3 aaa}]"
                                    {:parse-string? true, :style :odr})
                        (catch :default e (str e)))))

It seems pretty clear that this test is about an odd number of things in a map. The test expects a failure from clj and for it to work with cljs. What you are getting with the newer version of planck is exactly what you would kind of want to see for cljs -- that is, it works the same as clj.

But, but, isn't it rewrite-clj that is parsing this?

Yes, here are some simple examples taken from that test that seem to illustrate what used to happen with planck 2.25.0.

Here it is in clj:

zprint.core=> (czprint "[{k 1 g 2 c 3 aaa}]" {:parse-string? true})
[{aaa, c 3, g 2, k 1}]
nil
zprint.core=> (czprint "[{k 1 g 2 c 3 aaa}]" {:parse-string? true :style :odr})
Execution error at zprint.zprint/get-sexpr$fn (zprint.cljc:157).
Unable to parse the string '[{k 1 g 2 c 3 aaa}]' because of 'java.lang.IllegalArgumentException: No value supplied for key: aaa'.  Consider adding any unallowed elements to {:parse {:ignore-if-parse-fails #{ <string> }}}

The same thing in cljs:

zprint.core=> (czprint "[{k 1 g 2 c 3 aaa}]" {:parse-string? true})
[{aaa, c 3, g 2, k 1}]
nil
zprint.core=> (czprint "[{k 1 g 2 c 3 aaa}]" {:parse-string? true :style :odr})
[{aaa, c 3, g 2, k 1}]
nil

The difference in these two examples is the :style :odr, which causes zprint to try to do an sexpr on the parsed thing. So you don't have any trouble parsing it, but you will throw when I try to do an sexpr on it in clj. But not in cljs with the old planck.

Apparently you used to not throw on the sexpr in the previous version of planck, and now you do? Which isn't a bad thing, all things considered. It is probably a good thing. But what might have changed to make rewrite-clj do that?

[lread]

Hmmm... thanks for the clarifications and questions.
I haven't poked around at all yet, but will do a little digging sometime soon too.

[lread]

So here's my rewrite-clj tests for invalid maps:

https://github.com/clj-commons/rewrite-clj/blob/0d9df58372735ee144a11555c00e02fb00bcb960/test/rewrite_clj/parser_test.cljc#L218-L227

And I documented this understanding as well.

So I think you are telling me an sexpr was non-throwing on an unbalanced Clojure maps with the previous version of planck? I might have known that once, but I don't remember knowing it.

[lread]

Okay I've learned enough brew to switch between planck versions. 🙂

Here's a validation of your understanding:

Plank 2.25.0 - can sexpr an unbalanced map

$ plk                                                                                                                                                                                                                                    
ClojureScript 1.10.597
cljs.user=> (require '[rewrite-clj.parser :as p])
nil
cljs.user=> (require '[rewrite-clj.node :as n])
nil
cljs.user=> (def x (p/parse-string "{:a}"))
#'cljs.user/x
cljs.user=> (n/sexpr x)
{:a nil}
cljs.user=> {:a}
Syntax error reading source at (<cljs repl>:1).
The map literal starting with :a contains 1 form(s). Map literals must contain an even number of forms.
cljs.user=> 

And plank 2.26.0 - cannot sexpr an unbalanced map

$ plk                                                                                                                                                                                                                                                    
ClojureScript 1.10.914
cljs.user=> (require '[rewrite-clj.parser :as p])
nil
cljs.user=> (require '[rewrite-clj.node :as n])
nil
cljs.user=> (def x (p/parse-string "{:a}"))
#'cljs.user/x
cljs.user=> (n/sexpr x)
Execution error (Error) at (<cljs repl>:1).
No value supplied for key: :a
cljs.user=> {:a}
Syntax error reading source at (<cljs repl>:1).
The map literal starting with :a contains 1 form(s). Map literals must contain an even number of forms.

[kkinnear]

Wow, caught me replying. Yes, that is what I think is happening.

Overall, probably a good thing, yes? Brings the two environments into alignment.

[kkinnear]

The question is, how do we clean up the tests. I'm at least a couple of weeks if not more away from a new release. I have just had two releases in quick succession. The simple thing to do is for me to upgrade my planck, though I haven't yet looked to see if that is possible for my environment, and then fix my tests to be correct for that environment. Then after 1.2.3 comes out, we would be in alignment. I'll look into that. But that isn't going to help your testing situation at the moment.

[kkinnear]

Near as I can tell, there is now a more or less standard macOS ARM version of planck, so I'll plan to upgrade my planck and fix my tests prior to releasing zprint 1.2.3. I hope that I don't have to use your instructions for how to downgrade it, but I expect it will work -- as you have already shown me the only two tests that fail. Thanks for moving forward. I tend to just keep using the tools that work and don't always look for the latest ones.

[lread]

I raised this issue more in the spirit of informing you of a potential issue for zprint.

I've adapted rewrite-clj canary testing to disable zprint cljs tests for now.
I'll turn them back on when this is resolved, whenever that is.

[lread]

In case you need them, more brew lessons:

After following my brew instructions, you can switch:

1. from 2.26.0 to 2.25.0 via: brew unlink planck@2.25.0 && brew link planck
2. from 2.25.0 to 2.26.0 via: brew unlink planck && brew link planck@2.25.0

It all seems rather awkward, and there might be better ways.

[kkinnear]

Thank you for letting me know about this, I really appreciate it. Thank you also for the brew lessons. I don't use it often. While I agree it seems awkward, it also seems simple! Thanks again!

I'll leave this issue open to ensure that I actually upgrade planck and fix the tests.

[lread]

Overall, probably a good thing, yes? Brings the two environments into alignment.

Yeah, I agree. Feels like a cljs bug fix.

Not to bore you, but here's the crux of the difference:

Planck 2.25.0

ClojureScript 1.10.597
cljs.user=> (apply hash-map [:a])
{:a nil}

Planck 2.26.0

ClojureScript 1.10.914
cljs.user=> (apply hash-map [:a])
Execution error (Error) at (<cljs repl>:1).
No value supplied for key: :a

I expect this is not a planck thing at all, but a ClojureScript thing.

If I compare the current version of cljs:

$ clj -Sdeps '{:deps {org.clojure/clojurescript {:mvn/version "1.11.4"}}}' -M -m cljs.main                                                                                                                                                                                                                         
ClojureScript 1.11.4
cljs.user=> (apply hash-map [:a])
Execution error (Error) at (<cljs repl>:1).
No value supplied for key: :a

To an early 1.10 release:

$ clj -Sdeps '{:deps {org.clojure/clojurescript {:mvn/version "1.10.126"}}}' -M -m cljs.main                             
Compiling client js ...
Serving HTTP on localhost port 9000
Listening for browser REPL connect ...
To quit, type: :cljs/quit
cljs.user=> (apply hash-map [:a])
{:a nil}

We see the same difference.

[kkinnear]

Wow, thanks. That's very clear!

[lread]

This seems like the actual cljs change: CLSJ-3324

[kkinnear]

Thanks, that certainly nails down what is going on!

[kkinnear]

I've upgraded my planck (and therefore the version of Clojurescripit), and changed my tests to match. So you should be able to re-enable the Clojurescript tests now. Thanks for pointing out the change!

On a related note (related because of rewrite-clj V1 and rewrite-cljs), you might give the zprint issue #231 a look. Turns out that rebel-deadline in Figwheel uses rewrite-cljs, which messed up zprint. I think I figured out a reasonable fix.

[lread]

Congrats! I'll give it a go soon and letcha know!

For rebel-readline, I expect explicitly excluding the rewrite-cljs dep might do the trick?

[lread]

All zprint planck tests pass for me on macOS! Woot!
On GitHub Actions I use Ubuntu which installs the latest planck which is still at v2.2.25!
And zprint planck tests fail seem to fail for v2.2.25.
I'll go poke the planck repo and see if they plan to release 2.2.26 binaries for ubuntu.

[kkinnear]

Wow, isn't that interesting about planck. It looked like Ubuntu and macOS were the platforms of choice. I had no idea that the macOS version would be ahead of the Ubuntu version.

Regarding rebel-readline -- excluding rewrite-cljs is what I had figured out to solve the problem with zprint. It took me a while to get there, as it happens.

So, now zprint works with Figwheel both with and without rebel-deadline (as long as you exclude rewrite-cljs when using rebel-deadline). What I'm a bit fuzzy on is what happens to the parts of Figwheel that, presumably, depended on rewrite-cljs? It looks like rebel-deadline is using a very old version of cljfmt, 0.5.7, which is way before it was upgraded to use rewrite-clj V1.

I sort of assumed that the reason why zprint wasn't happy with rewrite-cljs (which it used to use), was because the rewrite-cljs namespaces were different than the rewrite-clj V0 namespaces (for reasons I never quite understood). So I had a bunch of code that looked in one place for :clj and another place for :cljs. If all of those surmises are correct, then excluding rewrite-cljs should mean that something in cljfmt didn't get connected up right, I would think. But there are no obvious errors from missing rewrite-cljs. There are two warnings, but something about (ua/isChrome), which doesn't seem related (and I think was independent of the exclusion).

But it does work with zprint just fine, so I'm considering that a win!

[lread]

I had no idea that the macOS version would be ahead of the Ubuntu version.

Me neither! 🙂

What I'm a bit fuzzy on is what happens to the parts of Figwheel that, presumably, depended on rewrite-cljs

Theoretically, rewrite-clj v1 should step in as a drop-in replacement for rewrite-cljs.

[kkinnear]

I don't think there is more to do here. If I'm wrong, please re-open the issue. Thanks!

[lread]

I think you are right, @kkinnear.
This is more of a planck issue than a zprint one.

I did take a whirl at getting a new release working for planck but came to the feeling that maybe the javascript engine (which planck uses) on Ubuntu is 20.04 is problematic.
See planck-repl/planck#1087, planck-repl/planck#1088.

But planck should work on ubuntu 22.04 (will still need to wait for a new release of planck).
See planck-repl/planck#1089.

Once Ubuntu 22.04 is out of beta on GitHub Actions, I'll try to remember to suggest a new release of planck over at the planck repo. After that, I'll update my canary tests for zprint at rewrite-clj.