Skip to content

Add datatype tests for empty#85

Merged
jeaye merged 3 commits into
jank-lang:mainfrom
rafonsecad:empty-datatypes
Jun 13, 2025
Merged

Add datatype tests for empty#85
jeaye merged 3 commits into
jank-lang:mainfrom
rafonsecad:empty-datatypes

Conversation

@rafonsecad
Copy link
Copy Markdown
Contributor

Adding empty tests on records as @NoahTheDuke suggested

Comment thread test/clojure/core_test/empty.cljc Outdated
Comment on lines +30 to +31
(defrecord Record [field])
(deftype Type [field])
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.

Each test should try to use as few other clojure.core vars as possible, since those vars may not exist on whichever dialect is running test. On top of that, I'm not sure if every major Clojure dialect implements defrecord and deftype.

I think it's good thinking to add these, but I'm currently thinking this is doing too much for the empty test. Anyone have thoughts on that?

@rafonsecad @NoahTheDuke

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yeah, that could be too much for empty test. In the other hand, we could add when-var-exists for defrecord and deftype to avoid the test from failing if those datatypes are not yet implemented

Copy link
Copy Markdown
Contributor

@NoahTheDuke NoahTheDuke Jun 12, 2025

Choose a reason for hiding this comment

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

definterface and defstruct aren't generic and are generally not included by dialects, but defrecord and deftype seem to have broad support (clojurescript, clojuredart, babashka, basilisp). I wouldn't think defrecord is any more special than range.

If you feel it necessary to bracket, then this portion of the test could be wrapped in (when-var-exists clojure.core/defrecord (testing "records" (defrecord Foo ...)))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Given the somewhat rigid structure of the tests, I also have no idea where such a combination test would go if not here.

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.

Good thoughts, folks. Yeah, let's use a when-var-exists for each of these vars. Mainly, I want these tests to also serve as an onboarding ramp for new dialects (like jank), so ideally each test has the smallest footprint it can.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@jeaye ok I just updated the pull request. Let me know if we need another change

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.

@rafonsecad So, we need to keep everything under the first when-var-exists check. Our new when-var-exists checks will be adding a new testing for records and a new testing for types. Know what I mean?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@jeaye yeah that makes sense. check again please

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.

Looks good to me. @NoahTheDuke?

@NoahTheDuke
Copy link
Copy Markdown
Contributor

great!

@jeaye jeaye merged commit b22ae6b into jank-lang:main Jun 13, 2025
2 checks passed
@jeaye
Copy link
Copy Markdown
Member

jeaye commented Jun 13, 2025

Thanks, @rafonsecad!

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.

3 participants