-
Notifications
You must be signed in to change notification settings - Fork 18
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
Standardise task options #23
Comments
The noise and extra time spent testing extra namespaces is certainly frustrating, and The kicker is that these tools are oriented around the classpath, and it's not straightforward to discriminate based on something like a "test" directory - that information has been lost by the time these tasks run. Also as you mention, there's no restriction about which files you define tests in - there are even things like Since your interest is portable suites, let's consider solutions that don't involve changing
Generic functions like these are simple, composable and portable, so I'll happily add them here and see if they have legs and fit your use case. Would also take PRs here to:
But not interested in those partial solutions enough to bother implementing myself. |
Thanks for your answer. I'll think about it within the next few days for better understanding what would be a clean solution. |
yes, it is, especially within a TDD workflow. I'm not a TDD practitioner, but I know they would judge unacceptable anything longer than 1 or 2 seconds.
Sure, I could do all of this from the outside of
what does it mean excludes for parity ? thanks |
The value of putting in a library is that the approach could mature and then find it's way into the tasks themselves if it proves popular and reliable. The code is just complex enough to be a chore adding to every project.
Just mean adding The biggest issue with a "solve via computing task arguments" approach is the |
You could write a task that scans the fileset for test namespaces and puts them into a list inside an edn file. This way they would be recomputed by watch and downstream tasks could pick them up. Maybe this creates some UX challenges but I think this is how you'd do it with Boot. |
|
|
@martinklepsch my issue with that approach is that downstream tasks need to know about these files, it's not really "open/closed". Would be interesting if there was some way to graft over a task's options. I guess this could be done with a task decorator? Or maybe this is a terrible idea 😄 |
Something like this: (defn wrap-task [task-fn args-fn]
(fn [next-task]
(fn [fileset]
(let [args (args-fn fileset)
task (task-fn args)]
((task next-task) fileset))))) Could wrap that around |
Not sure if I understand your decorator suggestion but you can supply On Fri, Dec 18, 2015 at 12:36 PM Chris Truter [email protected]
|
Have updated the PR to use the
Do you mean changing tasks so that they have an additional "function" version of given argument, eg. |
@magomimmo let me know what you think about this setup for opting into the kinds of conventions you suggested |
Looking at https://github.com/seancorfield/boot-expectations which has just been released, it's using an A note on |
@crisptrutski your with-ns function is smart, but is something unusual to be used by a boot user while composing tasks in the simplest possible way within the build.boot file. In some way, it’s exposing the user to an internal problem of the tasks components she’s using. from a user point of view I would expect: a) the b) by default, the I know that http://blog.jayfields.com/2010/08/clojuretest-introduction.html http://blog.jayfields.com/2010/08/clojuretest-introduction.html What I would like is something like this:
Takes into account all namespaces defined in the source-paths only (I’m still have to understand why someone uses
as above
A very short note on (defn ns-from-dir
"Given a directory, return a list of the namespaces given by files within it."
[dir]
(->> (file-seq (File. dir))
(map #(.getPath %))
(filter src-file?)
(map #(file->ns (relativize dir %)))))
Couldn’t been implemented as trandusers for speed? I’ll take a look at the Sean solution you cited. thanks a lot for you efforts and patience |
@crisptrutski @martinklepsch I looked at the Sean cited One more thing. there should be a way to keep the JS running in the auto mode (i.e. this has to do more with |
Agree, I was just trying to present a "works today" solution include
Would recommend taking this up directly on the boot-clj repo, as you’re out in the far colonies here :) I don’t have a strong opinion on this either. Generators like tenzing and saapas give you the starting-kit, and it's nice to be able to take off battery packs when you don't need them. Part of what put me of Leiningen is the “banana comes with gorilla and rainforest accessories” experience.
The grail of having the runner dynamically determine the tests using metadata and the power of clojure (eg. find just namespaces with test-vars, and names like the project), that gives the most power. Checking for tests via regexes or walking the AST seems absurd. You'll want to campaign for extra power (regexes being the starting point) up at There are also libraries that skip out I don’t agree about making the use of the project name. This convention is broken too often in my experience. Prefer being explicit here, although respective the project boundary properly is a must.
Agree that having separate regexes for namespaces and vars is odd, and the option is most valuable on the namespace. Sifting particular vars out does make sense to me though. eg. when triggered by watch, rerun only tests that failed on last attempt, or all files if everything passed. Would be great to just support including a
Yes I think so, some examples: filtering out slower tests, or tests for a different platform than you’re using (eg. *nix vs windows)
It could, although the initial Actually leaning towards just using https://github.com/clojure/tools.namespace/blob/6b19f942802f3b35059bb4acfa4791ef1717b8a9/src/main/clojure/clojure/tools/namespace/find.clj#L79, which is slightly slower but handles more edge cases. It looks even slower than my implementation, but I think that’s justified for the extra robustness. |
Thanks for putting a fire under this stuff, @magomimmo, there's a lot of room for improvement with this library. I don't have the luck of working with either Boot or CLJS at the moment, and I'm not even really keeping up with the communities, so this is a bit of a "repo under threat".
As a writer I think you're especially qualified to help push any of these agendas 😄 |
nobody is farway when making such fundamentals contributions like you do.
Myne was to have one JVM only and a more simple build file to be read
I agree on the details (regex are fragile). Here is a code snippet from Joe Fields article on this topic: (defmethod report :begin-test-ns [m]
(with-test-out
(when (some #(:test (meta %)) (vals (ns-interns (:ns m))))
(println "\nTesting" (ns-name (:ns m))))))
See above. main project segment is very fragile. The above Jay Fields's code snuppet would be better.
Sure. but the above code snippet seems to be the holygrail. |
you did great contributions. the availability of your lib is one of the reasons I started to take al look at
I'm confined in the colonies as you are :-), but I'll do my best. BTW, The boot community is very welcoming. |
Thanks for the kind words @magomimmo, much appreciated |
you're impressive @crisptrutski and last night you even got a big fish from the deep water! Your |
@crisptrutski @martinklepsch: thanks so much for the slack session last night! You were amazing. That said, talking about standardization/ergonomics, if I may say something as a typical thanks so much again! |
You're welcome! Not sure what you mean by |
A kind of harmonization between (most of the are already in place). Perhaps this is what you called parity somewhere. boot cljs -h
...
Options:
-h, --help Print this help info.
-i, --ids IDS Conj IDS onto the ids option.
-O, --optimizations LEVEL Set the optimization level to LEVEL.
-s, --source-map Create source maps for compiled JS.
-c, --compiler-options OPTS Set options to pass to the Clojurescript compiler to OPTS. boot test -h
...
Options:
-h, --help Print this help info.
-n, --namespaces NAMESPACE Conj NAMESPACE onto the set of namespace symbols to run tests in.
-e, --exclusions NAMESPACE Conj NAMESPACE onto the set of namespace symbols to be excluded from test.
-f, --filters EXPR Conj EXPR onto the set of expressions to use to filter namespaces.
-r, --requires REQUIRES Conj REQUIRES onto extra namespaces to pre-load into the pool of test pods for speed. and boot test-cljs -h
....
Options:
-h, --help Print this help info.
-e, --js-env VAL Set the environment to run tests within, eg. slimer, phantom, node,
or rhino to VAL.
-n, --namespaces NS Conj NS onto namespaces whose tests will be run. All tests will be run if
ommitted.
-s, --suite-ns NS Set test entry point. If this is not provided, a namespace will be
generated to NS.
-O, --optimizations LEVEL Set the optimization level to LEVEL.
-o, --out-file VAL Set output file for test script to VAL.
-c, --cljs-opts VAL Set compiler options for CLJS to VAL.
-x, --exit? Exit immediately with reporter's exit code. and probably I would add |
@magomimmo I think you'll be happy with the latest SNAPSHOT:
As you can see, no non-test namespaces and no 3rd part deps. Will prepare a stable release with this after it's seen a bit more use. Commandeering this task for the standardisation work, but would prefer to leave this as a contributor task 😉 |
Also need to juggle the short-hand CLI acronyms for these tasks, so they're all exposed naturally. |
@crisptrutski oh, yes. I love it. You rock! Yestarday I had an accident with my motorbike and I can't seat to much time in front of the computer, but I'll do my best for the standardization step. Thanks so much Chris, you really did a great job! |
Ouch! Glad you're OK - maybe a good time to try a standing desk 😄 |
@crisptrutski I just tried the new SNAPSHOT with the |
ops, I did not note the new |
The |
An empty test namespace is probably an edge case? I guess I'd suggest to include it in the output to make to rules for inclusion simpler and more easy to understand. E.g. someone trying to figure out why his test namespace isn't being ran will have an easier time.
|
@martinklepsch Easter eggs for xmas? Super geil 😉 ⛄ re: Confusing users with implicit exclusion - that's something I'm worried about too. The current behaviour is actually even more crude and unexpected than you imagine - if no namespace pattern(s) or literal(s) are provided, So it wouldn't confuse the user in the scenario you describe, but when it does strike the user, I imagine intense infuriation 😱 On the other hand I really prefer not to analyse source code or compile it and check vars.. Perhaps a warning about the default convention in the case where no tests are found (this style of naming is probably all-in or all-out) |
@martinklepsch happy holidays!!!! Your tasks was a great present. I appreciated a lot!
One thing I learned with customers is the following: if you tell them about an issue before they discovered it by themselves, you'll never loose one of them. A bold note in the
I try to use the trick by Jay Fields in the (ns test-helper
(:use clojure.test))
(defmethod report :begin-test-ns [m]
(with-test-out
(if (some #(:test (meta %)) (vals (ns-interns (:ns m))))
(println "\nTesting" (ns-name (:ns m)))))) It works, but it's slow too when compared with passing namespaces with the I love the pragmatic solution by @crisptrutski. Ok, It's based on convention, but it works beautifully in a TDD scenario. Could be this convention create any issue in a continuous deployment scenario?
Or eventually let the user overwrite the standard convention with her own convention? |
Hi @crisptrutski and @martinklepsch I think I completely misunderstood the (deftask tdd
"Launch a TDD Environment"
[]
(comp
(serve :handler 'modern-cljs.core/app
:resource-root "target"
:reload true)
(testing)
(watch)
(reload)
(cljs-repl)
(test-cljs :out-file "main.js"
:js-env :phantom
:namespaces '#{modern-cljs.shopping.validators-test})
(test :namespaces '#{modern-cljs.shopping.validators-test}))) I could satisfy my objective to have everything running in the same JVM which means a TDD environment with a browser connected REPL as well. Because I came from a previous execution of the following composed task (deftask dev
"Launch immediate feedback dev environment"
[]
(comp
(serve ;:dir "target"
:handler 'modern-cljs.core/app
:resource-root "target"
:reload true)
(watch)
(reload)
(cljs-repl)
(cljs))) the cljs compilation was already there (in the My personal opinion, but I could be very wrong on that, is that Could my goal be satisfy by only using the Sorry for my misunderstanding.... |
The behaviour of I'm not too sure what the interaction between There's no good reason why One thing to clarify is that The |
awesome! you're really smart! I just checked it now and it works like a charm.
I do not have an informed opinion on which used scenario of
uhm, it seems that the only problem I get in my case it happens when I cljs.user=> (t/run-tests 'modern-cljs.shopping.validators-test)
Testing modern-cljs.shopping.validators-test
Ran 1 tests containing 13 assertions.
0 failures, 0 errors.
WARNING: doo's exit function was not properly set
#object[TypeError TypeError: Cannot read property 'call' of null]
nil and cljs.user=> (t/run-all-tests)
Testing valip.core
Testing valip.predicates
Testing cljs.core
Testing modern-cljs.shopping.validators
Testing cljs.test
Testing cljs.pprint
Testing cljs.repl
Testing clojure.string
Testing valip.predicates.def
Testing cljs.reader
Testing modern-cljs.shopping.validators-test
Testing cljs.user
Testing clojure.template
Ran 1 tests containing 13 assertions.
0 failures, 0 errors.
WARNING: doo's exit function was not properly set
#object[TypeError TypeError: Cannot read property 'call' of null]
nil But if you have an auto-test running and an active CLJS REPL I do not see the need of running a test inside the REPL.
I'll think about it.
great!
I never tried to use in this way. I let you know if it will happen. Thanks a lot again for you awesome work. You really rock! |
@magomimmo thanks for the motivation here, I've had some time to mull things over, and decided to make the push to support almost all the params (with the same shortcuts) for these other tasks. The only exception being an exotic Will release these changes in a breaking boot-cljsSupport all options, with same names. Also support parsing relevant boot-testSupport Will take PR for #7 which would cover At this time do not consider boot-cljs-test extras
The The |
Hi @crisptrutski, thanks for your hard work! I'm in the process of writing a new tutorial of the modern-cljs series...as I publish that stuff I'll get into this...keep doing all your great coding! |
Now that @Deraen is helping out, expect drastic improvements soon 😄 If you get your next iteration out before we release |
Hi @crisptrutski, just wondering are you still working towards release 0.3.0? As wondering if the |
Hey @mattfreer - things came to a halt here based on a drastic shrinkage in my free time, lack of (perceived) urgency from users, and lack of external contribution. I've dropped all the context on the |
Microwaved the branch and releasing. Once it's seen some real world usage and I've updated the README will cut the Here's the new argument summary:
The two options discussed above that did not make it (yet?)
|
@crisptrutski thanks for the update, I'm using this branch now. Will let you know of any issues. |
@crisptrutski |
I can't really understand why both
boot-cljs-test
andboot-test
run all project namespace when the-n
option is not set to a specific test namespace. Is it not a waste of time, especially in a TDD workflow scenario?I know that to test
private
symbols you need to test them inside the defining namespace, but I never met a source code doing that. On the contrary, most of the time people create a test directory layout like the following:where all the application tests are confined. IMHO it would be better to have an option to say where the test namespaces are located and interpret the absence of the
-n
option as "run all the namespace in the test directories. Is my opinion so wrong?The text was updated successfully, but these errors were encountered: