Skip to content

initial tests for clojure.core/merge-with#896

Open
perrygeo wants to merge 6 commits into
jank-lang:mainfrom
perrygeo:mp/merge-with
Open

initial tests for clojure.core/merge-with#896
perrygeo wants to merge 6 commits into
jank-lang:mainfrom
perrygeo:mp/merge-with

Conversation

@perrygeo
Copy link
Copy Markdown

@perrygeo perrygeo commented May 24, 2026

Testing clojure.core/merge-with

Manually tested with

  • JVM
  • Clojurescript
  • Clojure CLR
  • Babashka
  • Basalisp (fails, see comment on nil handling)
  • PHEL (I don't have access to PHP tooling at the moment)

Dialect Differences

  • Clojurescript returns a nil in the zero-arg case. Clojure etc throw an exception.
  • Clojurescript: numeric keys have different equality semantics than the JVM.
  • Basalisp returns nil on (merge-with second-arg nil {}), rest return {}

Anything else I can do to improve this? Let me know. This is a really fun way to get to know the clojure standard library!

Resolves #361

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented May 24, 2026

CLA assistant check
All committers have signed the CLA.

@E-A-Griffin
Copy link
Copy Markdown
Collaborator

E-A-Griffin commented May 24, 2026

basilisp and phel status is unknown, are the core 4 langs sufficient?

They are yeah, @jeaye disabled them for the time being, that being said noting how they fail is always nice for their respective maintainers to be aware of (and potentially fixed)

Copy link
Copy Markdown
Collaborator

@E-A-Griffin E-A-Griffin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you use the following reader conditional order: https://github.com/E-A-Griffin/clojure-test-suite/blob/e00fe2b8ff87fcf02020664cc57f11bbf4448904/doc/writing-tests.md#reader-conditional-order? This PR isn't merged yet but @jeaye and I agreed on it prior so going to try to maintain it going forward

Comment thread test/clojure/core_test/merge_with.cljc Outdated
(testing "zero arg behavior"
;; NOTE: clojurescript differs from the rest here, bug?
#?(:clj (is (p/thrown? (merge-with)))
:cljs (is (nil? (merge-with))) ; TODO how to suppress warning
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this TODO for?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I run bb test-cljs, everything passes but the output contains this verbose warning:

...
Ran 223 tests containing 4957 assertions.
0 failures, 0 errors.
===============================================
[:test] Build completed. (324 files, 323 compiled, 1 warnings, 4.22s)

------ WARNING #1 - :fn-arity --------------------------------------------------
 File: /Users/matthew/projects/clojure-test-suite/test/clojure/core_test/merge_with.cljc:15:30
--------------------------------------------------------------------------------
  12 |      (testing "zero arg behavior"
  13 |        ;; NOTE: clojurescript differs from the rest here, bug?
  14 |        #?(:clj     (is (p/thrown? (merge-with)))
  15 |           :cljs    (is (nil? (merge-with))) ; TODO how to suppress warning
------------------------------------^-------------------------------------------
 Wrong number of args (0) passed to cljs.core/merge-with
--------------------------------------------------------------------------------
  16 |           :default (is (p/thrown? (merge-with)))))
  17 | 
  18 |      (testing "`nil`ish behavior"
  19 |        (is (nil? (merge-with second-arg nil)))
--------------------------------------------------------------------------------

Since we're explicitly testing the zero-args case, is there a way to silence this?

Comment thread test/clojure/core_test/merge_with.cljc Outdated
Comment thread test/clojure/core_test/merge_with.cljc Outdated
Comment thread test/clojure/core_test/merge_with.cljc
@E-A-Griffin
Copy link
Copy Markdown
Collaborator

I think sorted-map tests would also be nice

@perrygeo
Copy link
Copy Markdown
Author

Thanks for the review @E-A-Griffin ! The reader conditional ordering actually matters, TIL. I added tests for sorted-map and cleaned up a bit.

The cljs zero-arg case is the only real question mark. I've noted the difference in the top PR description. The warning still bothers me though, hoping to find a way to suppress that.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a new clojure.core compliance/characterization test namespace covering merge-with, with some dialect-conditional expectations (notably ClojureScript’s zero-arg behavior).

Changes:

  • Introduces a new merge-with test file with coverage for nil handling, collision semantics, fold direction, duplicate-key invocation, sorted-map preservation, and nested-map merging.
  • Adds negative tests around non-map inputs and exception behavior (with dialect conditionals).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread test/clojure/core_test/merge_with.cljc Outdated
Comment thread test/clojure/core_test/merge_with.cljc Outdated
@jeaye jeaye requested a review from dgr May 25, 2026 17:50
Copy link
Copy Markdown
Member

@jeaye jeaye left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

Comment thread test/clojure/core_test/merge_with.cljc Outdated
Comment thread test/clojure/core_test/merge_with.cljc Outdated
Comment thread test/clojure/core_test/merge_with.cljc
Comment thread test/clojure/core_test/merge_with.cljc
(:require [clojure.test :as t :refer [deftest is testing]]
[clojure.core-test.portability #?(:cljs :refer-macros :default :refer) [when-var-exists] :as p]))

(defn second-arg [_a b] b)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can delete this fn and just use second.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

second takes one arg and returns the 2nd element. I guess we could write #(second [%1 %2]) but not sure if that's an improvement.

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.

clojure.core/merge-with

5 participants