Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

clojure.test #200

Open
wants to merge 30 commits into
base: main
Choose a base branch
from
Open

clojure.test #200

wants to merge 30 commits into from

Conversation

frenchy64
Copy link
Contributor

@frenchy64 frenchy64 commented Jan 29, 2025

Workarounds noted in comment at the top of clojure/test.jank.

Needed clojure.core/ns-interns from clojure.core, but I had to add the function name to every (throw "TODO: port") statement to figure that out.

@frenchy64 frenchy64 changed the title WIP: Clojure test WIP: clojure.test Jan 30, 2025
Copy link

codecov bot commented Feb 3, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 58.37%. Comparing base (617968a) to head (f3ce443).
Report is 63 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #200      +/-   ##
==========================================
- Coverage   58.55%   58.37%   -0.19%     
==========================================
  Files         158      159       +1     
  Lines       19587    19936     +349     
==========================================
+ Hits        11470    11637     +167     
- Misses       8117     8299     +182     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@frenchy64 frenchy64 marked this pull request as ready for review February 5, 2025 05:39
@frenchy64 frenchy64 changed the title WIP: clojure.test clojure.test Feb 5, 2025
compiler+runtime/test/bash/module/clojure-test/pass-test Outdated Show resolved Hide resolved
@@ -4094,7 +4094,7 @@
c. Returns true or false"
[c x]
;; (. c (isInstance x))
(throw "TODO: port"))
(throw "TODO: port instance?"))
Copy link
Member

Choose a reason for hiding this comment

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

It's fine to change all of these, but it wasn't needed to find your error. For that, I'd just hop into gdb and tell it to stop when an exception is thrown.

gdb --args ./build/jank run foo.jank
> catch throw
> run
... wait for an exception to be thrown and gdb will stop ...
> backtrace
... see where you are ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aha, so (byte 1) will give #2 0x00007fffe8df885d in clojure_core_byte_3074_1 ().

Very instructive thanks.

Copy link
Member

@jeaye jeaye Feb 6, 2025

Choose a reason for hiding this comment

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

Not quite. clojure_core_byte_3074_1 means clojure.core/byte <gensym count> <arity 1>. This is for the function itself, not a call to it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. I pointed it out because AFAICT this is the only frame that gives a hint of which function needs implementing. Is there a way to configure source mapping? e.g.,

;;foo.jank
(ns foo)
(defn -main [] (byte 1)) 
#8  0x00007ffff33850e0 in foo.main_1_0 >>here>> at foo.jank:2 <<here<< ()

Copy link
Member

Choose a reason for hiding this comment

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

There will be, but it's a large feature which involves a lot of LLVM IR work. Perhaps I know the right guy for it, though. I'll reach out to him.

compiler+runtime/src/jank/clojure/test.jank Outdated Show resolved Hide resolved
@frenchy64 frenchy64 requested a review from jeaye February 6, 2025 02:23
@frenchy64
Copy link
Contributor Author

Putting this back to draft actually, clojure.test/are doesn't work yet.

@frenchy64 frenchy64 marked this pull request as draft February 6, 2025 05:29
@frenchy64 frenchy64 changed the title clojure.test WIP: clojure.test Feb 6, 2025
@frenchy64
Copy link
Contributor Author

Opened and worked around #237

WDYT about the approach here of gradually adding the clojure-test suite? Just requires a deps.edn to be added to clojure-test: {:paths ["test"]}.

@frenchy64 frenchy64 marked this pull request as ready for review February 6, 2025 05:47
@frenchy64 frenchy64 changed the title WIP: clojure.test clojure.test Feb 6, 2025
@jeaye
Copy link
Member

jeaye commented Feb 6, 2025

Opened and worked around #237

WDYT about the approach here of gradually adding the clojure-test suite? Just requires a deps.edn to be added to clojure-test: {:paths ["test"]}.

I think we can gradually add for now, but note that the clojure-test suite is specifically designed to support missing vars. It will skip them. So, we should just #_ out our unimplemented defns and then run all tests.

Syntactically, I think the only thing we're missing is big number support. Will need to get someone on that.

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.

2 participants