Additional edits for ClojureCLR tests #107

dmiller · 2025-09-05T18:35:11Z

No description provided.

dmiller · 2025-09-05T18:57:56Z

The failing test in shuffle does not happen on my system.

You'll want to take a close at the changes. I had to do some non-trivial rearrangement in the string library tests.

Also, for with-out-str, I had to introduce a helper function from clojure.test-helpers. I could not figure out how to :require or :use that library. (Both ClojureJVM and ClojureCLR use it in their test suites.)

jeaye · 2025-09-05T19:12:27Z

Thanks, David! I'll leave this for @E-A-Griffin to help with, both for the copied code (can we avoid it?) and the failing test.

E-A-Griffin · 2025-09-05T19:20:44Z

Will run tonight

E-A-Griffin · 2025-09-05T19:21:06Z

deps-clr.edn

@@ -0,0 +1,11 @@
+{:deps {}
+
+   :aliases


weird spacing and indentation

fix as you wish.
a copy of a copy of a copy from somewhere someone else used the test-runner

E-A-Griffin · 2025-09-05T19:21:10Z

deps-clr.edn

+   {:test
+     {:extra-paths ["test"]
+	  :extra-deps {io.github.dmiller/test-runner {:git/sha "c055ea13d19c6a9b9632aa2370fcc2215c8043c3"}}
+	  ;; :main-opts {"-m" "cognitect.test-runner" "-d" "test"}


could we drop this dead code?

E-A-Griffin · 2025-09-05T19:21:39Z

test/clojure/core_test/byte.cljc

      0    0
      127  127
      1    1N
      0    0N
-      -1   -1N
+      #?@(:cljr [] :default [-1   -1N])


spacing is weird

I had just inserted the original code in the :default case.
Format as you see fit, for sure.

E-A-Griffin · 2025-09-05T19:21:51Z

test/clojure/core_test/byte.cljc

      1    1.0M
      0    0.0M
-      -1   -1.0M
+      #?@(:cljr [] :default [-1   -1.0M])


spacing is weird

E-A-Griffin · 2025-09-05T19:22:04Z

test/clojure/core_test/byte.cljc

           1    1.1M
-           -1   -1.1M]))
+           #?@(:cljr [] :default [-1   -1.1M])]))


spacing is weird for each of these

E-A-Griffin · 2025-09-05T19:25:24Z

test/clojure/core_test/byte.cljc

+         (is (thrown? Exception (byte -128.000001)))
+         (is (thrown? Exception (byte -129)))
+         (is (= 128 (byte 128)))
+         (is (= 127(byte 127.000001)))


spacing is weird

E-A-Griffin · 2025-09-05T19:32:04Z

test/clojure/core_test/with_out_str.cljc

+     (defn platform-newlines [s] (.Replace ^String s "\n" nl)))        ;;; .replace, add type hint
+   :clj
+   (let [nl (System/getProperty "line.separator")] 
+    (defn platform-newlines [s] (.replace s "\n" nl)))


style nitpick but I think it'd be cleaner to have the var defined with a defn at the top level for all implementations and then change the body based on platform, i.e.

(defn platform-newlines [s] #?(:clj (let [nl (System/getProperty "line.separator")] (.replace s "\n" nl)) :cljr (let [nl Environment/NewLine] ;;; (System/getProperty "line.separator")] (.Replace ^String s "\n" nl))) ;;; .replace, add type hint :default s)

E-A-Griffin · 2025-09-05T19:32:40Z

test/clojure/string_test/blank_qmark.cljc

-      (is (#?(:cljs true? :cljr true :default false?) (str/blank? " ")))
-      (is (#?(:cljs true? :cljr true :default false?) (str/blank? "\u2007"))))
+      (is (#?(:cljs true? :cljr true? :default false?) (str/blank? " ")))   
+      (is (#?(:cljs true? :cljr true?  :default false?) (str/blank? "\u2007"))))


spacing is weird here

E-A-Griffin · 2025-09-05T19:37:49Z

deps-clr.edn

+	  ;; :main-opts {"-m" "cognitect.test-runner" "-d" "test"}
+	  :exec-fn cognitect.test-runner.api/test
+	  :exec-args {:dirs ["test"]
+	              :patterns [".*test.*"]}}}


I don't think this will work, most namespaces don't have test in the name

you are probably right, though i think all tests were running for me. I think you can just leave out the :patterns.

I'll try it again but it didn't work for me when I tried this last night

this works, thanks!

E-A-Griffin · 2025-09-06T03:15:45Z

@dmiller this is what I got

Ran 172 tests containing 3949 assertions.
40 failures, 24 errors.

E-A-Griffin · 2025-09-06T03:31:56Z

@dmiller I can push the changes if you're able to give me write access to your fork so I can push updates to your branch (otherwise I think I'd need to fork your fork which seems excessive)

dmiller · 2025-09-06T14:08:07Z

Invitation sent.

…

On Fri, Sep 5, 2025 at 11:32 PM Emma Griffin ***@***.***> wrote: *E-A-Griffin* left a comment (jank-lang/clojure-test-suite#107) <#107 (comment)> @dmiller <https://github.com/dmiller> I can push the changes if you're able to give me write access to your fork so I can push updates to your branch (otherwise I think I'd need to fork your fork which seems excessive) — Reply to this email directly, view it on GitHub <#107 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAUVZMUABP6P4EIDYU7G7L3RJIUFAVCNFSM6AAAAACFX6MH6SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTENRQGMYDGOJVGI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

E-A-Griffin

There's still some failing CLR tests (on my end, not sure if @dmiller has any failures) but I think we can clean those up later, I'm happy to take a further look in a follow up but this is blocking multiple PRs so I think better to get it through and make incremental progress

E-A-Griffin · 2025-09-06T19:37:15Z

@jeaye ^ left my approval with a note, if you want to take a look

jeaye · 2025-09-07T21:02:03Z

@jeaye ^ left my approval with a note, if you want to take a look

Thanks, Emma. We can't merge failing tests, since we need CI on main to be passing (and we need CI on this PR to be passing prior to merging). If there's anything currently failing which we plan on tackling in the future, we can disable that code with a #_ or similar, along with a comment regarding the issue and intention to fix.

dmiller · 2025-09-07T21:57:56Z

That sounds like a plan. I'm on call to help resolve issues. At present I have only seen intermittent failure in tests in add-watch and tap. Given that they are intermittent, I'm wondering about some timing problems in those tests. But otherwise, I have only passes.

…

On Sun, Sep 7, 2025 at 5:02 PM Jeaye Wilkerson ***@***.***> wrote: *jeaye* left a comment (jank-lang/clojure-test-suite#107) <#107 (comment)> @jeaye <https://github.com/jeaye> ^ left my approval with a note, if you want to take a look Thanks, Emma. We can't merge failing tests, since we need CI on main to be passing (and we need CI on this PR to be passing prior to merging). If there's anything currently failing which we plan on tackling in the future, we can disable that code with a #_ or similar, along with a comment regarding the issue and intention to fix. — Reply to this email directly, view it on GitHub <#107 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAUVZI4RNTVJQEAB7JHPR33RSMODAVCNFSM6AAAAACFX6MH6SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTENRUGA2DCNJTGE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

E-A-Griffin · 2025-09-07T23:49:22Z

@dmiller if the tests are passing on your end then it's probably an issue on my local

E-A-Griffin · 2025-09-09T14:08:34Z

@jeaye okay, this time for real I think it's ready

jeaye

Great job, both of you!

dmiller · 2025-09-09T20:25:54Z

I might have made a little more work for the two of you than intended. It was a fair amount of editing. As folks continue to add to the testing suite, consider me on call to make sure things work in ClojureCLR.

…

On Tue, Sep 9, 2025 at 2:56 PM Jeaye Wilkerson ***@***.***> wrote: Merged #107 <#107> into main. — Reply to this email directly, view it on GitHub <#107 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAUVZMQ2HC3LQZIAC5C5O33R4PFDAVCNFSM6AAAAACFX6MH6SVHI2DSMVQWIX3LMV45UABCJFZXG5LFIV3GK3TUJZXXI2LGNFRWC5DJN5XDWMJZGU4TMNRUGI3TCNQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

Additional edits for ClojureCLR tests

076c2d4

E-A-Griffin reviewed Sep 5, 2025

View reviewed changes

test/clojure/core_test/byte.cljc Outdated

1 1.0M

0 0.0M

-1 -1.0M

#?@(:cljr [] :default [-1 -1.0M])

Copy link

Contributor

E-A-Griffin Sep 5, 2025

Choose a reason for hiding this comment

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

spacing is weird

E-A-Griffin reviewed Sep 5, 2025

View reviewed changes

Fix formatting

66957f7

E-A-Griffin added 4 commits September 6, 2025 13:30

Cleanup with-out-str

029111c

Cleanup blank_qmark

23711b3

Cleanup byte

e1bd5e1

Cleanup 2

98e1f59

E-A-Griffin approved these changes Sep 6, 2025

View reviewed changes

E-A-Griffin added 3 commits September 8, 2025 09:39

Fix whitespace

d33f41f

Fix cljs shuffle

1b6759f

Fix clj

45d7295

jeaye approved these changes Sep 9, 2025

View reviewed changes

jeaye merged commit a54a567 into jank-lang:main Sep 9, 2025
2 checks passed

Additional edits for ClojureCLR tests #107

Additional edits for ClojureCLR tests #107

Uh oh!

Conversation

dmiller commented Sep 5, 2025

Uh oh!

dmiller commented Sep 5, 2025

Uh oh!

jeaye commented Sep 5, 2025

Uh oh!

E-A-Griffin commented Sep 5, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

E-A-Griffin commented Sep 6, 2025

Uh oh!

E-A-Griffin commented Sep 6, 2025

Uh oh!

dmiller commented Sep 6, 2025 via email

Uh oh!

E-A-Griffin left a comment

Choose a reason for hiding this comment

Uh oh!

E-A-Griffin commented Sep 6, 2025

Uh oh!

jeaye commented Sep 7, 2025

Uh oh!

dmiller commented Sep 7, 2025 via email

Uh oh!

E-A-Griffin commented Sep 7, 2025

Uh oh!

E-A-Griffin commented Sep 9, 2025

Uh oh!

jeaye left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dmiller commented Sep 9, 2025 via email

Uh oh!

Uh oh!