Fix test/glojure/test_glojure/numbers.glj failures #73
Conversation
After this patch: $ ./bin/linux_amd64/glj test/glojure/test_glojure/numbers.glj Testing glojure.test-glojure.numbers Ran 42 tests containing 2744 assertions. 0 failures, 0 errors. Before this patch: $ ./bin/linux_amd64/glj test/glojure/test_glojure/numbers.glj Testing glojure.test-glojure.numbers FAIL in (nil) (:) expected: (= vals (map wrapped inputs)) actual: (not (= [\� \ \ \ \� \� \� \� \�] (\� \ \ \ \� \� \� \ \))) FAIL in (nil) (:) expected: (= vals (map wrapped inputs)) actual: (not (= [255 0 1 127 255 255 255 255 255] (255 0 1 127 255 255 255 0 0))) FAIL in (nil) (:) expected: (= vals (map wrapped inputs)) actual: (not (= [-1 0 1 127 32767 -1 -1 -1 -1] (-1 0 1 127 32767 -1 -1 0 0))) FAIL in (nil) (:) expected: (= vals (map wrapped inputs)) actual: (not (= [-1 0 1 127 32767 2147483647 9223372036854775807 9223372036854775807 9223372036854775807] (-1 0 1 127 32767 2147483647 9223372036854775807 -9223372036854775808 -9223372036854775808))) FAIL in (nil) (:) expected: (= vals (map wrapped inputs)) actual: (not (= [-1 0 1 127 32767 2147483647 9223372036854775807 9223372036854775807 9223372036854775807] (-1 0 1 127 32767 2147483647 9223372036854775807 -9223372036854775808 -9223372036854775808))) Ran 42 tests containing 2744 assertions. 5 failures, 0 errors.
jfhamlin
left a comment
jfhamlin
left a comment
There was a problem hiding this comment.
Thanks for putting this together! I remember these cases being tricky when I first tried to run this in CI, and that might have been one of the reasons I left my CI PR hanging :). I left a comment with some context and a couple options. Let me know your thoughts on how we can fix this robustly.
| STDLIB_TARGETS := $(addprefix pkg/stdlib/glojure/,$(STDLIB:.clj=.glj)) | ||
|
|
||
| TEST_FILES := $(shell find ./test -name '*.glj') | ||
| TEST_FILES := $(shell find ./test -name '*.glj' | sort) |
| [char [:error (char 0) (char 1) (char 127) (char 32767) :error :error :error :error]] | ||
| ;; In go, char == rune, which is equivalent to int32 | ||
| [unchecked-char [(Char -1) (Char 0) (Char 1) (Char 127) (Char 32767) (Char math.MaxInt32) (Char -1) (Char -1) (Char -1)]] | ||
| [unchecked-char [(Char -1) (Char 0) (Char 1) (Char 127) (Char 32767) (Char math.MaxInt32) (Char -1) (Char 0) (Char 0)]] |
There was a problem hiding this comment.
Unfortunately, for extreme inputs, the behavior turns out to be system-dependent; the unchecked casts intentionally map (nearly) directly to go casts to rune (there's a stop at int64 in-between). These tests are a modified version of Clojure's numbers tests, and on the JVM you can expect consistent behavior. This operation does not behave consistently across systems in Go. For instance, running on my m1 mac, the result is (Char -1).
The right approach here might be one of:
- Introduce a new
:skipexpectation; ignore cases whose expectation is:skip. Leave comments with reasoning. - Support system-dependent expectations
2 is interesting, but there's no great way to be exhaustive. 1 seems adequate. What do you think?
There was a problem hiding this comment.
Honestly I just wanted to not see the failures for these when running make test. Now I don't and the CI is green.
I'd be fine with just disabling these tests for now. Maybe comment them out with TODO text explaining why.
BTW, I'm close to submitting a PR that fixes issues with basic sort and sort-by. Just adding more tests.
I'm in the Clojure slack, if you want to chat more easily :)
|
Slightly different fix merged in #80 |
2 participants
I'm not sure why tests were failing since they pass here: https://github.com/glojurelang/glojure/actions
But this fixed it for me locally and still passing here: https://github.com/ingydotnet/glojure/actions
After this patch:
Before this patch: