Implement Clojurescript port for self-hosted Clojurescript.

[kkinnear]

Here it is. I hope I have all of the pieces together -- there are a lot of moving parts in this pull request! I've done the code and made a pass at documenting it as well, so in addition to this rather long list of things here, the documentation (new file doc/getting-started-cljs.md and the end of the README.md) might be useful for you. I've tried really hard to make sure it is all together, but it wouldn't surprise me if I've missed something somewhere.

Notes I made while working on the code:

0. Very impressive use of macros! I learned a lot just by working on this. Both the basic expectations ones and the testing ones. Amazing!

1. I have made all of the source changes by trying to change as few lines as possible, and have hand formatted the things I did change. The point is to have a diff show the bare minimum set of changes, so you can easily see what I have done. Given that I am a big proponent of source formatting, this has been a challenge, to be sure. I also didn't want to intrude on your approach to source formatting (though I expect that I could configure zprint to do it "your way"). But I tried to
   minimize changes. The formatting of what you see is not ideal from my standpoint, but the point is to minimize your effort for review.

2. One can't really do run-time requires in cljs:

    Calls to `require` must appear at the top-level.   

So if we are to allow humane-test-output, it has to be there all the time. I think we should, since every bit of help is worth it.

The alternative would be to do more of the diffing like classic expectations does and output it ourselves. Which, if you think it would be useful, is something I would consider contributing (as I already have code to handle strings, discussed much later in this list). If we did that, we could dump HTO in cljs and maybe in clj as well. But for now, the cljs version requires HTO and hacks it to work with the locally created :diffs. See below for details.

The same goes for spec -- no way to really use it only if it is available. So, the cljs version requires spec to be there. Which isn't a big deal.

3. The doc for humane-test-output for cljs says that it just works without activation. Not so as I could tell -- you have to at least call (pjstadig.util/define-fail-report) to get it to work in cljs.

Here is the code for that, which you may care about for the next item:

(defn define-fail-report [] 
  #?(:clj (defmethod report :fail [& args] 
            (with-test-out 
            (report- (first args)))) 
    :cljs (defmethod cljs.test/report [:cljs.test/default :fail] 
            [event] 
            (report- (p/convert-event event))))) 

4. The cljs/cljc version of humane-test-output handles :diffs differently than the clj version (i.e., it ignores whatever you pass in), so I had to kind of hack a patch into it as part of activating it. See the cljs code at "(def humane-test-output?" in test.cljc. It would be reasonable to get them to change that I suppose. This change seemed really awful when I first hacked it in, but over time I became more tolerant of it. I'm not sure why. I suppose because I didn't want to wait for two libraries to change just to get it to work at all. And because it is so simple as to be transparent, I suppose.

5. I had to use a number of planck routines to get this to work, so it is clearly wedded to planck. I suspect that lumo, the other self-hosted cljs environment, has similar routines and that it could be made to work there as well. I think the macro situation is so different in regular cljs that I don't have any idea how to make this work, since without a cljs environment around when you expand the macros, none of the resolve (or find-var) stuff is going to work.

6. Exceptions exist and can be thrown in Javascript. You can throw pretty much anything in Javascript. There is no Throwable class in cljs to distinguish things that can be thrown from anything else. This makes it hard to distinguish a potential Exception from anything else that shows up as "expected" and know when to generate the (is thrown? ...) form.

At present the only exception you can look for (i.e., where (is thrown? ...) is used) is js/Error. That isn't substantially different from what classic expectations does with cljs exceptions, I think.

I don't know if this is a major limitation or not. I suspect that I could figure out how to support :default as the expected, and in that case I would accept anything that was thrown, but not by using (is thrown? ...), rather by doing a (try (catch :default ...)) which is a special cljs thing to catch anything that was thrown. I can work on this if you want (or we could do that in a later update).

7. I spent way too long dealing with the fact that classes (such as they are in Javascript) and functions both return true to (fn? ...). Plus, there is no (class? ...) in cljs. This makes distinguishing predicates from classes really hard. I have managed to separate them by looking at the results of printing them, as functions print as "#object[Function]", where classes do not. I could, instead, have looked at the length of the arglist, as classes have a null arglist and predicates to not. But however it is done, this distinction is vital. Awful, I think, but vital.

8. use-fixtures -- this is a macro in cljs.test, and apparently we just really want the cljs.test behavior. So we pass through directly to the cljs.test macro using our own macro. The approach for clj using apply doesn't work in cljs.

9. from-clojure-test: This is a challenge in cljs, where you can't modify metadata on vars. Planck has a way to do this, and it seems to work for clj as well, so I went with that. The arglists just don't come over, and I don't know why or if that will become a problem. Doesn't seem to be problem so far.

10. Using the cljs version of humane-test-output with a modern cljs version produces lots of warnings. HTO doesn't really do anything wrong, it uses what appears to be a legitimate call to a function in cljs.pprint, but that call uses private functions which (relatively) recently have started to throw warnings during cljs compilation. These warnings can be disabled, which I have done by passing some options to planck. Another reason to dump HTO, I suppose, long term.

11. Getting the clj tests to work for cljs was at least as much work as getting test.cljc modified, perhaps more. The only remaining pain is that the spec testing doesn't work if the (s/def ...) is in the same compilation unit as the (deftest ... (expect ...)) form. To work around this, I stuck it at the end of test.cljc for cljs instead of making yet another file in the testing area. (It also doesn't work if it is in test_macros.cljc). Everybody who uses expectations.clojure.test for cljs will therefore get a single (s/def ...) spec defined, but since it is namespaced it doesn't seem like a terrible thing. I expect that another file in the testing area would also solve this if you prefer. Seems odd to have a file with one line in it, but that would almost certainly work.

12. I changed the artifact-id to be cljc-test from cloure-test" and learned that the artifact-id isn't really related to the namespace, which was news to me. Usually they are related so I assumed that was necessary, which it appears it is not. In our discussion on github, I thought that you implied that changing the artifact-id and not the namespace would be your favored solution to the restrictions in doo for self-hosted cljs. I probably would have changed the namespace to follow the artifact-id so that they went together, for no reason other than trying to reduce surprises for users (and because I incorrectly thought it was required). But this works, and seems reasonable now that I've lived with it a while. I would be a a lot of work to change the namespace everywhere, and disrupt current users as well, so this seems like a good compromise (though the artifact-id change will disrupt current users a bit as well when they upgrade).

13. The end of README.md has instructions for running the cljs tests, as well as how to get a planck cljs REPL working with expectations/clojure-test loaded inside if you want to experiment with it.

14. Getting cljdoc to work was, as usual, interesting. I love cljdoc, but I find it challenging to get it to analyze cljs files without a lot of work. This time was no exception. The planck.core namespace is built into planck, but ... the cljs analysis phase complains that it needs planck.core. Sigh. So, I eventually found a planck.core that actually seems to work in cljdoc on its own, and supplied that and now cljdoc works. But we don't really want planck as a dependency for expectations/clojure-test, since it is only necessary to get cljdoc to work. And there is no way to tell cljdoc to use this special thing and not have it be part of the dependencies for the .jar for everyone (near as I can tell, anyway). After a lot of trial and error, emphasis on the error, it seems like the only approach I can find using deps.edn is to edit planck into pom.xml with "scope provided". Having done that, it turns out that HTO needs the same treatment, so they are both edited into pom.xml by hand, with "scope provided". Thus, HTO isn't brought in for clj operations unless you want it, and it must be supplied by the user for cljs operations since I could find no way to have a .jar file with different deps for clj and cljs let alone a way to specify this in deps.edn. This means that cljs users have to specify HTO on their own, it doesn't get included as a dependency from the .jar.

This all seems to work, but leaves the pom.xml file something that needs to be edited by hand, but that seems like the way of things in the deps.edn world in any case, so I suppose it is ok. It is already the case that the artifact-id doesn't show up anywhere except pom.xml, which is slightly worrisome. I'm just not used to hand-editing the pom.xml, I suppose. I think of it as something derived from more user friendly configuration information.

The bottom line is, I have run cljdoc locally and it works with the current documentation. But don't remove planck or HTO from the pom.xml or it will stop working. I put [planck "2.23.0"] in deps.edn under the :cljdoc alias, even though there is actually no need for it to be there, but it worried me that having it only in the pom.xml would mean it would just get lost.

15. I have made two other changes to my local version of expectations/clojure-test for both clj and cljs which I find helpful (and almost necessary for my use case). I have not included them in this pull request, but will add them for your review when (if?) we get the current Clojurescript changes integrated::

a) I have added the "actual" form (before evaluation) to the msg that comes out when a failure occurs. This is useful for two reasons:

1. Clojurescript doesn't yet do file names and line numbers pretty much at all (though that is apparently coming), so that when a test fails, it can be mighty hard to figure out which test is failing. With the "actual" form, a simple text search is easy.

2. I can cut the "actual" form from the failure report and paste it into a repl and reproduce the failure without even having to find it in the source for the tests. This was also possible with the classic expectations, and I missed that capability a lot.

Given that I have 945 tests that run in cljs and 1002 in clj, this is a big win in my environment.

b) I have a lot of tests that expect a string as output, and it is often very long string which wraps onto many multiple lines. The HTO handling of string diffs is pretty much =/not=, which is a long way from the classic Expectations "matched/diverges" approach to strings, and pretty useless for strings that wrap.

So I have implemented the equivalent support to the classic Expectations string handling and stuck that in the msg when the expected and actual are both strings and they don't compare. This helps me out a lot, and almost certainly wouldn't hurt anyone else. The code is short and simple and common to both clj and cljs.

That's it for now.

[seancorfield]

Wow, this will take me a while to digest and respond to but at first glance, this looks workable "as-is" so I'll spend some time reviewing it at work tomorrow and will probably merge it and then we can hash some of the rest out in a discussion here and decide what, if any, changes need to be made before cutting a release. Big, big thank you for all the work you've put in on this!

[seancorfield]

1. re: formatting -- I'm curious as to what sort of differences in formatting you found? I just let Atom/Chlorine/Parinfer indent my code as it wants so I assumed it was fairly standard formatting.

2/3/4. re: top-level requires/HTO -- I'm surprised cljs doesn't allow that (I guess I'm going to be surprised by most differences!) but as long as HTO remains an optional dependency in the clj version, I'm happy -- and you'll be the only cljs user of the library for now so as long as you're happy always using HTO, I'm also happy. HTO makes some failure reports a lot worse so I don't actually use it (but I know some folks love it -- and one of the biggest complaints about clojure.test itself is how poor the failure reporting is). This is probably an area I'll tackle in 2.0.0 (although I might wait until clojure.test

5. re: planck -- I'm happy to wait until someone using lumo et al file an issue about this :)

6. re: js/Error -- I'm happy to wait until someone complains or sends a PR :)

7. re: classes vs functions -- Ouch! Is the (expect ClassName actual) form even useful in JS/cljs?

8. re: use-fixtures -- Yup, in 2.0.0 (which is what develop is heading toward), I wanted to have use-fixtures support the richer functionality from the cljs version.

9. re: from-clojure-test -- I'll take a look at this: it took me a while to figure out an approach in clj which worked best with code complete/assist in editors so if this changes the behavior for clj, I'll probably want to make more of the function conditional, using my original version in clj and using your version in cljs.

10. re: HTO -- see 2/3/4 above :) I want smarter failure reporting in the underlying library!

TBC...

[seancorfield]

11. re: s/def location -- that's definitely an interesting difference! It would probably be better to put the spec in its own file in the test tree but you're still going to have conditional weirdness to make sure the clj side can run on Clojure 1.8 (pre-spec) so it probably wouldn't be much of an improvement.

12. re: artifact ID -- Yup, the group/artifact ID is purely for deployment and dependencies and is completely unrelated to the namespaces inside the library. As far as I'm concerned, it's a bug in doo and it's being overly zealous about excluding JAR names but, as you say, it's not realistic to expect doo to fix this in the short term, and indicating cljc-style compatibility with both clj and cljs in the artifact name isn't a bad thing. And changing the artifact ID in a dependency when you bump the version is a much smaller change than making folks change every require!

13. re: testing with planck -- I didn't have it installed and I'm still waiting for brew install planck to complete(!) but then I will try it.

14. re: pom.xml -- Thanks for the heads up on this. My normal process for deployment is clojure -Spom && clojure -A:jar ... && git status so I'll have to watch out for the pom.xml changes there since clojure -Spom will remove Planck and HTO I suspect... I may experiment and see if there's some better workflow. I'll talk to Alex Miller about it.

15a) re: actual form -- the difference (from Classic Expectations) is to be more compatible with clojure.test which in turn means better support for Cursive's test running machinery (I worked with Colin Fleming on this a while back). As I recall, Cursive relies on the post-evaluation actual form to provide better contextual diffs. Maybe we need a user-controllable execution mode for this at some point. Again, it really comes down to clojure.test's failure reporting being poor :|

15b) re: string diffs -- that seems like a reasonable enhancement to have built-in so I'd be interested in a PR for that.

[seancorfield]

Re: 13. My version of macOS is too old to install planck on via brew (since it requires an up-to-date version of icu4c which won't build with my old XCode install). So I have no way to test the cljs side of things. Looks like doo will use node for testing, which is how I deal with this for HoneySQL -- but I guess the dependencies here on planck are too deep-rooted for that to be possible?

[seancorfield]

Re: 1. I let Parinfer "fix" the formatting -- 6c6adbe -- Just FYI. I don't know whether this makes it better or worse from your p.o.v. :)

[seancorfield]

Re: 13, part 2. On my Windows laptop tonight and installed planck on WSL1 and the cljs tests run beautifully!

Also, I pointed our large test suite at work to the develop branch locally and everything still works, so that feels like a good sanity check that nothing broke in the clj side of the house.

Big thanks again for all this work!

[kkinnear]

Thanks for all of the responses, particularly the most recent one where you can now run the cljs tests!

I went to sleep last night trying to figure out how to make this work in non-self-hosted cljs, and near as I can tell, the places in the expect macro where you use resolve and I use planck.core/find-var won't work in traditional cljs, because the macro expansion happens in clj, and the things to resolve just aren't around at all. It is possible that those decisions could be moved to run-time in =?. I haven't explored that, in part because I'm not entirely clear on exactly how =? integrates with assert-expr and just exactly what the "run-time" is when that gets executed -- and I was hoping to avoid having to dig imore deeply into those interactions. And now I don't have to, so thank you again for finding some way to run planck! Trying to make this work in regular cljs is not uninteresting, but right now I'm tired of beating my head against that particular wall.

Other responses:

One thing I've noticed is that while I want expectations/clojure-test to be able to run my tests in both clj and cljs, I'm sure someone will want to use it to run tests just in cljs, and I need to keep thinking of them when doing the code and docs.

2/3/4/HTO. I'm not an HTO user, I'm a convert from classic expectations. I figured "more is better", so I figured that we should include HTO. I would be a fan of dumping HTO over time and moving more of the classic expectations-style failure comparisons into expectations/clojure-test, since that would simplify a lot and give even better diagnostics. I'll update and send you a PR for my beginning effort on that for strings.

7. classes/functions -- I suppose expecting a class is useful in cljs, probably about as much as it is for clj. I don't ever use this form of expect, so I don't have any useful opinions here (besides that it is hard to implement in cljs, and even harder to have tests that work in both environments using classes).

8. :re clojure-test I'm glad you can check to see how this works with various editors, I have one environment and while it works for me, it doesn't partake of any of the fancy helpful features that might be affected by this change. I think the code I put in has the same end result in clj as the previous code, but it might not. Thanks for checking this out!

9. (s/def location: do you want me to move this to a separate file?

10. :re pom.xml If you use clj -Spom as part of your workflow, you must edit the pom.xml for the version number, right? The version number isn't stored anywhere else that I could find. And now, in addition to planck and HTO, the pom.xml needs to have the artifact-id changed, since I suppose clj -Spom will not know about expectations/cljc-test. Seems like the pom.xml now has a lot of information not held elsewhere. I'm surprised that someone hasn't done a clj tool that gets this stuff from some additional keys in deps.edn. But I couldn't find anything.

11. My additional changes. I'll do a PR for the string compare. In both of my changes, I append my information in the "msg" field, I don't change the "actual" or "expected" fields, so I expect that neither should upset Cursive, though as I don't use it, I don't know how sensitive it is. If the string compare doesn't mess it up, the "actual" form shouldn't mess it up either. I would think.

Source formatting -- I'll see if I can find a way to get you an out-of-band copy of test.cljc formatted by zprint and you can see what I think of as nice formatting. Not to try to change your formatting approach, just FYI.

Thanks for accepting this PR!

[seancorfield]

2/3/4/HTO: There's a string-differ in Classic Expectations that could be used as-is (it's referenced in the docstring for expectations.clojure.test/functionally) -- expectations/strings-difference. I don't know how that compares to your code.

9. Might as well move it -- and reuse it in both the clj and cljs versions. test/expectations/clojure/test_spec.clj perhaps?

10. clojure -Spom only updates the dependencies section. It will generate a skeleton pom.xml file if you don't have one but then it won't touch group/artifact/version. And, yes, the version (and tag) items need to be updated manually as part of preparing a release, so the provided dependencies are just something I need to watch out for since my usual workflow won't quite apply.

11. Sounds good -- see 2/3/4/HTO above :)

Re: source formatting: also sounds good. I'll know as soon as I open the file whether Parinfer will want to mess with it or not :)

Make sure future PRs are based against the latest develop since I've put in a bunch of changes since merging this (including letting Parinfer "fix" the indentation in every file!).