Skip to content

Conversation

daveliepmann
Copy link
Contributor

No description provided.

This is arguably an implementation detail and not worth testing. Included for
completeness nonetheless.
@daveliepmann
Copy link
Contributor Author

daveliepmann commented Sep 5, 2025

Test fails because babashka implements protocols with multimethods. This is a good canary case: do we care if protocols are fn? or is that an implementation detail and we should remove that test (and possibly multimethods too)?

@jeaye
Copy link
Member

jeaye commented Sep 5, 2025

Test fails because babashka implements protocols with multimethods. This is a good canary case: do we care if protocols are fn? or is that an implementation detail and we should remove that test (and possibly multimethods too)?

To me, this is an implementation detail which is good to identify and document. I think your test for this, and your reasoning, is good. Let's skip the test for :bb in a reader conditional, with a comment explaining that babashka uses multimethods for this. Sound good?

@borkdude for vis.

@borkdude
Copy link
Contributor

borkdude commented Sep 5, 2025 via email

@jeaye
Copy link
Member

jeaye commented Sep 5, 2025

Looks overly specific. Why not test for IFn instead?

These are the unit tests for clojure.core/fn?. We're just exercising different inputs to that to characterize which things in Clojure are functions.

@borkdude
Copy link
Contributor

borkdude commented Sep 5, 2025 via email

@jeaye
Copy link
Member

jeaye commented Sep 5, 2025

What are you basing these tests on, documentation or just poking and seeing which things are fn? In Clojure? If the latter is the method of coming to a test suite like this I think you may end up testing overly specific things

We're supporting Clojure, ClojureScript, Clojure CLR, babashka, and working on jank support, right now. The tests are written to target all of them and point out any discrepancies or idiosyncrasies we find. These are good things to indentify.

We would all say that each of these dialects is "Clojure", but nobody knows exactly what "Clojure" is. This test suite is one way to help us figure out how similar (and dissimilar) each of these dialects are. The fact that babashka uses multi-methods for protocols is not a problem, but it's a good thing to note, since it has implications for how those protocols interact with the rest of the system.

@borkdude
Copy link
Contributor

borkdude commented Sep 5, 2025 via email

@jeaye
Copy link
Member

jeaye commented Sep 5, 2025

The fact that bb uses multimethods for protocol fns is an implementation detail that may change any day and not something anyone should rely on. If someone were to make a bb compat test suite for an alternative bb written in COBOL, this isn’t something I would like them to imitate :)

I understand your point. Thanks for taking the time to weigh in and discuss.

@daveliepmann, I think borkdude brings up fair criticism that this is beyond the scope of the behavior fn?. Our goal is to see how the function works, not necessarily find all of the things in Clojure which are fns. It helps to do the latter in order to do the former, which brings us here, but perhaps we've gone too far.

@borkdude
Copy link
Contributor

borkdude commented Sep 5, 2025

I think for protocols it would be good to add a test around the behavior of protocols though, including satisfies? etc (possibly also instance? but this is already a difference between CLJ and CLJS). So more like: are protocols working as expected in this dialect of clojure.

@borkdude
Copy link
Contributor

borkdude commented Sep 5, 2025

And stuff like multi-arity functions in protocols, etc.

@jeaye
Copy link
Member

jeaye commented Sep 5, 2025

I think for protocols it would be good to add a test around the behavior of protocols though, including satisfies? etc (possibly also instance? but this is already a difference between CLJ and CLJS). So more like: are protocols working as expected in this dialect of clojure.

We're just focused on unit testing core functions (i.e. all of clojure.core, clojure.string, etc). I agree that it'd be great to have a test suite for functional testing dialect features, but this is something which dialects are more likely to have. From what I've seen, and from speaking with Alex Miller, nobody has a thorough unit test suite for the Clojure standard library.

Not until this project, anyway!

@borkdude
Copy link
Contributor

borkdude commented Sep 5, 2025

Thanks for clarifying. I still think it would be great to have such tests since I've fixed quite a number of corner cases in SCI while developing protocols and other features. But clojure.core fns are a great way to start.

As Erik Assum has hinted, https://github.com/borkdude/speculative may also be of some value/inspiration to this project, since that project contains specs for clojure.core functions. It's not in any way comprehensive though. The issue tracker of that project contains some interesting discussions with Alex as well.

Anyway, thanks for this test suite and thank you @daveliepmann for improving it!

@daveliepmann
Copy link
Contributor Author

Agreed that it got too specific there. I removed the tests that rely on implementation details and left a comment explaining the reasoning. I feel much better about this namespace's testing scope now. Thanks for the discussion!

@@ -0,0 +1,34 @@
(ns clojure.core-test.fn-qmark
(:require clojure.core
Copy link
Collaborator

Choose a reason for hiding this comment

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

Requiring clojure.core here should be redundant

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No opinion — see b617559 and

Copy link
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.

Looks great, Dave! 💪

@jeaye jeaye merged commit 1e0a6f5 into jank-lang:main Sep 7, 2025
2 checks passed
@daveliepmann daveliepmann deleted the test-fnqmark branch September 8, 2025 08:57
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.

4 participants