probcomp / metaprob

An embedded language for probabilistic programming and meta-programming.
GNU General Public License v3.0
168 stars 17 forks source link

Document the fact that one must use `lein repl` to get the right classpath, and add a target that imports all the tests #151

Closed bzinberg closed 5 years ago

bzinberg commented 5 years ago

See https://github.com/probcomp/metaprob/issues/150#issuecomment-496029834.

alex-lew commented 5 years ago

I just did

$ clj
Clojure 1.9.0
user=> (require 'metaprob.examples.all)

with no errors. clj does look at deps.edn, so I'm not sure what the issue was here?

bzinberg commented 5 years ago

@alex-lew What output do you get when you run these two commands? Is test/ in your classpath both times?

alex-lew commented 5 years ago

Same as you. But I can run clj -Atest or clojure -Atest to run tests just fine. I think the instructions for running tests from the REPL are outdated (from when we used to use lein) -- it looks like @zane changed the other instructions here? https://github.com/probcomp/metaprob/commit/806e1c808b869595fafcbca9d3f814834d9dfe99#diff-b7d20b8f522e7e1162db38bb8ab03a08

I'm not sure how to run tests from the clj REPL, but maybe Zane can shed some light here -- and I think that's preferable to going back to lein (which we were trying to move away from, IIRC, but kept around for lein notebook).

bzinberg commented 5 years ago

Ah I see. Sounds like we should either remove the claim that tests can be run from within the REPL, or add test/ to the classpath so that they actually can be run from the REPL (or can they already and I'm missing something?). Is there some reason to not add test/ to the classpath?

alex-lew commented 5 years ago

Ah I see. Sounds like we should either remove the claim that tests can be run from within the REPL, or add test/ to the classpath so that they actually can be run from the REPL (or can they already and I'm missing something?). Is there some reason to not add test/ to the classpath?

Will defer to @zane or @joshuathayer on these q's, but share your intuition :)

zane commented 5 years ago

Sounds like we should either remove the claim that tests can be run from within the REPL…

The tests can be run from the REPL.

In order to run a test from the REPL you first need to require its namespace. In order for the namespace to be required successfully its source file and dependencies need to be on the classpath. The clj -C and -R flags can be used to achieve this.

Running a single test from the REPL:

$ clj -C:test -R:test              
Clojure 1.9.0
user=> (require 'metaprob.trace-test)
nil
user=> (metaprob.trace-test/nil-not-a-trace)
nil

Running all the tests in a namespace from the REPL:

$  clj -C:test -R:test 
Clojure 1.9.0
user=> (require 'clojure.test)
nil
user=> (require 'metaprob.trace-test)
nil
user=> (clojure.test/run-tests 'metaprob.trace-test)

Testing metaprob.trace-test

Ran 8 tests containing 33 assertions.
0 failures, 0 errors.
{:test 8, :pass 33, :fail 0, :error 0, :type :summary}

Running all the tests in all the Metaprob test namespaces from the REPL:

clj -C:test -R:test 
Clojure 1.9.0
user=> (require 'clojure.test)
nil
user=> (require 'metaprob.autotrace 'metaprob.code-handlers 'metaprob.compositional-test 'metaprob.distributions-test 'metaprob.expander 'metaprob.generative-functions 'metaprob.inference-test 'metaprob.prelude-test 'metaprob.syntax-test 'metaprob.trace-test)
nil
user=> (clojure.test/run-all-tests #"metaprob.*")
…
Ran 40 tests containing 124 assertions.
0 failures, 0 errors.
{:test 40, :pass 124, :fail 0, :error 0, :type :summary}
zane commented 5 years ago

Is there some reason to not add test/ to the classpath?

It's customary for the project classpath to include only the project source, not its test source, by default. This is because the default project classpath defines what will be passed along to downstream consumers. Consumers are generally only concerned with the source and dependencies necessary to use our project, not the source and dependencies necessary to develop it.

zane commented 5 years ago

Closing this as, as Alex mentioned, we are attempting to move away from Leiningen for the time being.