Add tests for partition#893
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new clojure.core/partition test namespace to cover expected behavior across its supported arities and validate laziness characteristics (issue #389).
Changes:
- Introduces a new
partition.cljctest file guarded by(when-var-exists partition ...). - Adds assertions for arities 2/3/4 including overlapping partitions, padding behavior, and empty/nil collection inputs.
- Includes checks that the result is a lazy seq and initially unrealized.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| (testing "arity 4 - (partition n step pad coll" | ||
| ;; Use padding for the last element | ||
| (is (= '((0 1 2) (1 2 3) (2 3 4) (3 4 :a)) (partition 3 1 [:a :a :a] (range 5)))) | ||
| (is (= '((0 1 2) (3 4 5) (6 7 8) (9 :a :a)) (partition 3 3 [:a :a :a] (range 10)))) |
There was a problem hiding this comment.
Potential test: What if pad collection is too small?
Potential test: What if pad collection is empty?
|
I believe this is ready to merge. |
|
I'm not convinced we want the TODO for the floating point value support, since I gather that the fn is intended to only accept integral values. CLJS just happens to support it since there's only one number type for JS. I think we should remove the floating point case, and the TODO. Keeping the big integer case is good, though, since it's an integral type. |
I think the test is good to keep but the TODO should be dropped as I don't think non CLJS are obligated to do any particular behavior with floats, that being said, I think CLJS' behavior is interesting and worth documenting here (could see this causing a real bug in the wild for someone) |
E-A-Griffin
left a comment
E-A-Griffin
left a comment
There was a problem hiding this comment.
I think we should add a test for (partition 0 ...) akin to what's shown here: https://clojuredocs.org/clojure.core/partition#example-57aa75fae4b0bafd3e2a04d3
E-A-Griffin
left a comment
E-A-Griffin
left a comment
There was a problem hiding this comment.
Mostly looks good, left some optional suggestions
Closes #389