Skip to content

Conversation

@willcohen
Copy link
Contributor

Please answer the following questions and leave the below in as part of your PR.

  • This PR corresponds to an issue with a clear problem statement.
    clojure.test missing for cherry #178, also dependent on Add clojure.test to cherry namespace mappings squint#778 (applied locally for purposes of this PR). I know it's a fairly big PR, so I tried to organize as a series of iterative commits with corresponding tests.

  • This PR contains a test to prevent against future regressions
    A number of tests, actually -- the async functionality re js-await is somewhat different from all the IAsyncTest stuff, so it's where I'm the least confident it's the right way to go, though it feels fairly idiomatic to me right now. My strategy has been to build up a cross_platform_test.cljc file, which bb integration-tests runs using jvm clj as well as node cherry cli, which can serve as documentation for how cherry's testing is similar to or different from clojure.test. The fixtures functionality does seem useful for a minimal implementation, so that's in. clojure.test and cljs.test do a lot more with defmethods and namespaced-based test findings, which I left out for this first version.

  • I have updated the CHANGELOG.md file with a description of the addressed issue.

@willcohen
Copy link
Contributor Author

(note that CI will be expected to fail right now given that it still requires an override to use squint's squint-cljs/squint#778)

@borkdude
Copy link
Member

Merged the squint PR

@willcohen
Copy link
Contributor Author

Thanks -- deps.edn modified accordingly.

@willcohen
Copy link
Contributor Author

Blah, I broke something in my last rebase. Sorry. I need to fix.

@willcohen
Copy link
Contributor Author

Actually -- it's still working locally on macOS. I'll need to debug why it's not working on CI.

@borkdude
Copy link
Member

Perhaps it's an advanced compilation issue?

@willcohen
Copy link
Contributor Author

Looks like it’s that I used clj instead of clojure, so it’s failing on not having rlwrap. Will fix and rebase.

@willcohen
Copy link
Contributor Author

That was it -- just the rlwrap. Ready for review @borkdude.

Note that the FAILS that print to the console during the tests are intended -- that's the test suite checking that cherry.test correctly fails under those four conditions in the verify tests. I think hiding them would require implementing piping test output to different places, which I assume may be out of scope for v1 here.

Also note that I'm handling the -main behavior by manually invoking it on cljs. Not sure if there's a better way to automatically invoke on the JS side.

@willcohen
Copy link
Contributor Author

Further testing: one thing I really don't like right now is https://github.com/squint-cljs/cherry/pull/181/files#diff-08adca3dde44605617eed55bdf8a519a9c22f52dcaa9faeaaecb95ee747ada11R2-R10 -- these refers are really unpleasant to expect users to do. Before this is considered ready, I'll need to do some more thinking about how all these things resolve so that this boilerplate isn't always required by end users.

@borkdude
Copy link
Member

Since this is fairly big PR I won't get this this before Christmas

@willcohen
Copy link
Contributor Author

Of course. It's gonna need some iteration anyway, so no hurry. Happy holidays!

@willcohen
Copy link
Contributor Author

@borkdude For whenever you're ready after the holiday, this is now substantially cleaned up -- no more messy refer stuff. This is now dependent on squint-cljs/squint#785, which is not reflected in deps.edn. This currently fails CI, but you can review its ability to pass CI by locally depending on the squint branch in that PR, and doing bb build && bb integration-tests.

@willcohen willcohen force-pushed the test branch 2 times, most recently from 33290d9 to b4fe35b Compare December 26, 2025 16:12
@willcohen
Copy link
Contributor Author

Went from +688 to +633 with some helper fns.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants