Skip to content

Add tests for >=#865

Merged
jeaye merged 5 commits intojank-lang:mainfrom
dgr:dgr-greater-equal
Apr 20, 2026
Merged

Add tests for >=#865
jeaye merged 5 commits intojank-lang:mainfrom
dgr:dgr-greater-equal

Conversation

@dgr
Copy link
Copy Markdown
Collaborator

@dgr dgr commented Apr 17, 2026

Closes #123

@jeaye jeaye requested a review from E-A-Griffin April 17, 2026 19:44
[clojure.core-test.portability #?(:cljs :refer-macros :default :refer) [when-var-exists] :as p]))

(when-var-exists >=
(deftest test->=
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.

nitpick, I think test>= is more clear since to me this looks like we're testing some function/macro ->=

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The template has a standard format: "test-" appended with function name. I'm going to leave this as is unless you have a lot of heartburn, just for consistency with every other test.

Comment on lines +44 to +49
true 0 0
true 1 1
true -1 -1
true ##Inf ##Inf
true ##-Inf ##-Inf
false ##NaN ##NaN ; ##NaN is never equal, even to itself
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.

indentation is weird here

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

(is (= true (apply >= 10 (reverse (range 10)))))
(is (= false (apply >= -1 (reverse (range 10))))))

(testing "negative tests"
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.

I think we'll want to scale back on these negative tests, since they're technically undefined behavior in Clojure. The docs string for >= says Returns non-nil if nums are in monotonically non-decreasing order, otherwise false. so there is an implication that all inputs must be nums.

Interestingly, the doc string also says non-nil, rather than true.

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.

Yeah "returns non-nil" is very oddly non specific

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I'll switch the tests to check for non-nil and false as opposed to true and false.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Also removed all negative tests for anything other than numbers and nil.

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.

Small things but looks good to me!

@E-A-Griffin
Copy link
Copy Markdown
Collaborator

E-A-Griffin commented Apr 17, 2026

One thing that I think could be a good add would be binary numeric comparisons for applicable hosts, Clojure, ClojureScript, ClojureCLR, bb for example have

user=> (>= 2r0110 2r0100)
true

@E-A-Griffin
Copy link
Copy Markdown
Collaborator

Also would be good to test negative Infinity

@jeaye
Copy link
Copy Markdown
Member

jeaye commented Apr 17, 2026

One thing that I think could be a good add would be binary numeric comparisons for applicable hosts, Clojure, ClojureScript, ClojureCLR, bb for example have

user=> (>= 2r0110 2r0100)
true

I like the out-of-the-box thinking, but in this case, 2r0110 is just parsed into a normal number and the behavior of >= is not different. We'd just be testing the parser, which is not the goal here.

@jeaye
Copy link
Copy Markdown
Member

jeaye commented Apr 17, 2026

Also would be good to test negative Infinity

Good thinking.

@dgr
Copy link
Copy Markdown
Collaborator Author

dgr commented Apr 19, 2026

Also would be good to test negative Infinity

There are multiple tests with ##-Inf, so I'm not sure what you're looking for here.

@dgr
Copy link
Copy Markdown
Collaborator Author

dgr commented Apr 19, 2026

One thing that I think could be a good add would be binary numeric comparisons for applicable hosts, Clojure, ClojureScript, ClojureCLR, bb for example have

user=> (>= 2r0110 2r0100)
true

I like the out-of-the-box thinking, but in this case, 2r0110 is just parsed into a normal number and the behavior of >= is not different. We'd just be testing the parser, which is not the goal here.

I agree with this. There's no added benefit as the syntax is just specific to the reader and it's all converted to integers by the time it reaches >=.

@dgr dgr marked this pull request as draft April 19, 2026 06:46
@dgr
Copy link
Copy Markdown
Collaborator Author

dgr commented Apr 19, 2026

Changing this to draft PR until I can change the tests to check for non-nil rather than true.

@dgr
Copy link
Copy Markdown
Collaborator Author

dgr commented Apr 20, 2026

Alright, I think this is ready to go now.

@dgr dgr marked this pull request as ready for review April 20, 2026 19:36
@jeaye jeaye merged commit ee60cc6 into jank-lang:main Apr 20, 2026
6 checks passed
@jeaye
Copy link
Copy Markdown
Member

jeaye commented Apr 20, 2026

Thanks, Dave!

@dgr dgr deleted the dgr-greater-equal branch April 20, 2026 22:51
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/>=

3 participants