lambdaisland / kaocha

Full featured next gen Clojure test runner
https://cljdoc.org/d/lambdaisland/kaocha/1.0.861/doc/1-introduction
Eclipse Public License 1.0
797 stars 83 forks source link

Support running clojure.spec.test.alpha/check #45

Closed RickMoynihan closed 5 years ago

RickMoynihan commented 5 years ago

It might be good to consider how to support generative testing suites through clojure.spec and a kaocha wrapper over the clojure.spec.test.alpha/check runner (or possibly by providing an alternative implementation).

Here's a proposal as to how this might work:

Firstly there are a few behaviours of check that have implications that will need to be considered.

  1. Calling the 0-arity (check) finds all loaded fdef specs and executes them. The difficult thing here is that the fdefs and specs need to be loaded already for them to execute. So we'd need to provide a mechanism to do this. fdefs are named by the vars of the functions they check, so there is no way to provide a naming convention feature to find them.

The easiest approach here is probably just to let the user provide a set of namespaces to load before we trigger check to locate and test the specs, through something like :kaocha/ns-patterns "myapp.*". Spec "tests" will likely need their own :kaocha.testable/type (I'd suggest :kaocha.type/clojure.spec.test.alpha.check to allow for alternative runners in the future). It's also worth bearing in mind there may not be a need for the equivalent of :kaocha/test-paths as specs commonly exist in the application namespaces.

  1. The 1-arity (check sym-or-syms) allows you to provide either a single symbol representing an fdef to check, or a set of symbols to check. This should be configurable via the tests.edn and command line.

  2. (check sym-or-syms opts) takes the symbol/set as with the 1-arity but also takes a map of test.check/quick-check options and :gen overrides. These should both be configurable in the config, however only the quick-check options should be settable via the command line (it doesn't make much sense to supply :gens via the commandline). In particular these include: the :seed option which can likely be set to the same seed kaocha uses, and :max-size which is required for constraining the size of inputs and is very important to set if you want performant tests. Additionally it is important that users can set specs additional :num-tests property here which specifies the number of random tests to generate for each fdef.

I think it may also be useful for users to be able to orchestrate a set of check tests where multiple check calls can be made with different settings for each of the above options within a single suite (as with :core-specs suite below:

#kaocha/v1
{:tests     ;; [1]
    [{:id    :core-specs
              :type :kaocha.type/clojure.spec.test.alpha.check
              :kaocha/ns-patterns "myapp.*"
              :source-paths ["src"]
              :checks [{;; :syms #{myapp.foo/bar myapp.foo/baz}  ;; omitting :syms means find all fdefs
                              :clojure.spec.test.check {:num-tests 100 :max-size 20 }
                              {:syms #{myapp.flib/flip myapp.flib/flop} 
                              :clojure.spec.test.check {:num-tests 10 :max-size 10 }
      {:id :slow-specs
            :type :kaocha.type/clojure.spec.test.alpha.check
            :kaocha/ns-patterns "myapp.*"
            :source-paths ["src"]
            :clojure.spec.test.check {:num-tests 1000 :max-size 50 }
            :checks [{:syms :all-fdefs}]]}]}

Note here that options can be specified at multiple levels, with the inner in :checks overriding those provided in the wrapping layer. I think a reason to do this is to avoid complecting the suite with the sizes you need to supply to a test etc. Some specs are much more sensitive to sizing parameters than others, and may for example not terminate without large heap settings.

I think any sizing/num-tests options provided at the command line though would override everything within the config and apply to the whole suite.

Finally we would need to consider reporting. check emits a lazy sequence of reports containing the following information

:spec       the spec tested
:sym        optional symbol naming the var tested
:failure    optional test failure
::stc/ret   optional value returned by test.check/quick-check

These would then need to be summarised and printed in human readable form. Additionally if any errors/exceptions occur we should report them nicely with context. We should parse an ::s/failure data if it's present and provide a human readable explanation of the problem:

The value for :failure can be any exception. Exceptions thrown by
spec itself will have an ::s/failure value in ex-data:

:check-failed   at least one checked return did not conform
:no-args-spec   no :args spec provided
:no-fn          no fn provided
:no-fspec       no fspec provided
:no-gen         unable to generate :args
:instrument     invalid args detected by instrument
WhittlesJr commented 5 years ago

This would be the holy grail of testing for me. Currently I'm using this: https://gist.github.com/kennyjwilli/8bf30478b8a2762d2d09baabc17e2f10

WhittlesJr commented 5 years ago

Oh, and to your last point, I assume you would want to use expound for the human-readable failure output, right?

WhittlesJr commented 5 years ago

Actually, I think I might just take a swing at implementing this myself, unless someone else is working on it currently.

plexus commented 5 years ago

By all means, go for it! Sorry I haven't been able to provide more input here, I don't have as much time for open source right now as I'd like.

Were you thinking of adding this directly to kaocha, or doing it as a plugin?

WhittlesJr commented 5 years ago

@plexus , this kind of feature strikes me as something that would go into kaocha directly, as I think it would warrant a new testable type as @RickMoynihan said. My impression of the existing plugins is that they seem to be ways to modify how tests are run, whereas this feature actually generates new tests. But your plugin system is really robust so it is probably possible to write it as a plugin, if you think that's better?

plexus commented 5 years ago

In that case it should be a separate library, look for instance at kaocha-cljs, kaocha-cucumber, or kaocha-midje, which all supply extra test types. A test type is a namespace that implements two multimethods.

I'm sorry to say that writing test types isn't as well documented as writing plugins, there are a number of things to watch out for to make sure they place nicely with all the plugins.

You can use this as a starting point, and use the other test type implementations as reference. Feel free to ping me if you have specific questions. https://cljdoc.org/d/lambdaisland/kaocha/0.0-418/doc/9-extending

WhittlesJr commented 5 years ago

Dang, in that case I'm not sure if I'll be able to share my work... I've been able to get around corporate IP policy for OSS by making submissions to existing open source libraries with existing open licenses, but if it's a new library written on company time, I don't think I can release it...

I've already made some decent progress, and I've refactored out a few functions in your code for re-use with the spec generative test types to stay DRY. I could share what I have as a PR and someone else could pick it up from there? (Or I could continue on my free time, but it would take a lot longer.)

I've found implementing the feature to be pretty easy going so far, your code is well-written.

plexus commented 5 years ago

How about you submit the PR, and I have a look and see if I either keep it inside the main kaocha artifact, or pull it out into a separate kaocha/kaocha-spec-check project? I'll give it a readme and a changelog, then it's my project and you can make more contributions there if you like.

WhittlesJr commented 5 years ago

Sounds good ;)

WhittlesJr commented 5 years ago

I'm currently debating on a couple of UI points:

  1. Is a plugin the best way to offer simplistic CLI arguments to this? That's what I have in my WIP PR above at the moment. (See kaocha.plugin.alpha.spec-check.)
  2. I kind of like @RickMoynihan's suggestion that CLI arguments would override all ::stc/num-tests and ::stc/max-size for all checks. Right now my plugin is pretty dumb and just adds a single check for :all-fdefs given the above arguments, but I guess functionally that's pretty much the same, right?
  3. Is a list of :checks the best way to "orchestrate a set of check tests where multiple check calls can be made with different settings for each of the... options within a single suite?" In the case of, say, a handful of fdefs that would need to be tuned to be faster, all the rest of your fdefs would not need to be touched. So, either you literally list every other fdef in the :syms of another entry in :checks, or we add the ability to sort of do an ":other-fdefs" that matches every symbol that wasn't specified in any other :checks.
WhittlesJr commented 5 years ago

To point 2, I realized that it is functionally not the same.

plexus commented 5 years ago

For 2 and 3 I'd need to play around with it first, I haven't used these fdef based generative tests in anger yet.

For 1 I think that's fine, as I mentioned in the PR review I've actually thought for a while that it would be useful to have test-types double as plugins, i.e. if you have a test suite of type :foo.bar/baz then we also try to load a plugin :foo.bar/baz if it exists. This is useful for extra command line flags, but also handling other special cases.

WhittlesJr commented 5 years ago

Thanks for the detailed feedback, I'll do my best to get it A.) working and B.) conformed to your requirements today. But from what you've seen, do you think that you want this in kaocha proper, or pulled out into another library?

WhittlesJr commented 5 years ago

Er sorry, I just noticed that you said you'd go over it yourself, so I'll stop making changes! I'll submit what I've done this morning though

plexus commented 5 years ago

Sounds good! I'm leaning towards pulling it out, since this would require an extra library (test.check) to be added to the main dependencies otherwise.

plexus commented 5 years ago

If you want to continue some more with it that's also fine by me. Seems it's not actually in a runnable state yet, so how about you continue until it (somewhat) runs, then I'll have a pass with some tweaks and comments, and then you can take it over again?

WhittlesJr commented 5 years ago

Can do. How about I also refactor it out into a separate library while I'm at it? If you make the kaocha/kaocha-spec-check project I'll submit the PR against it when it's working. But I'll still have a PR against kaocha for code reuse.

And speaking of, you said you didn't want me to use so much of :kaocha.type/ns's implementation for the spec fdefs in a given namespace. What would you suggest instead? I'm pretty sure the implementations would be almost interchangeable if you want to treat fdefs similarly to how you treat test vars (broken up by namespace).

plexus commented 5 years ago

I don't think one testable should directly call code from another testable. We can pull shared logic into utility namespaces like kaocha.testable and kaocha.result, but e.g. I think each testable should define its own ->testable function. It's fine if there's some duplication there. The upshot is that they can evolve independently, and that it's easier to reason about each in isolation.

It also means that loading one type does not mean we have to load the other type, since test types are only loaded if used in the test config.

How about I also refactor it out into a separate library while I'm at it?

Leave it here for now, it'll be easier to coordinate things, especially when we're also refactoring some of the existing kaocha code. I can pull it out easily before cutting a new release.

WhittlesJr commented 5 years ago

Understood. I'll do that.

WhittlesJr commented 5 years ago

Oh, also:

Should spec-check be my noun of choice, or should it be a spec-test-check, to match the clojure.spec.test.check keyword namespace used in clojure.spec.test.alpha? The abbreviation stc seems to be standard.

WhittlesJr commented 5 years ago

(I'm going with spec.test.check for now)

WhittlesJr commented 5 years ago

See #95 for a seemingly working implementation.