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
792 stars 81 forks source link

Change randomize? at any level #272

Open kennyjwilli opened 2 years ago

kennyjwilli commented 2 years ago

Problem

The majority of the time, I like running my tests with randomize? true. Rarely, I will have a case where running tests for a specific test suite or namespace would be beneficial. Typically, this is because an earlier test(s) do an expensive, mutative operation that I do not want to redo. This might feel close to a :once fixture. However, there can be an important difference -- the expensive operation is the thing we actually want to test. As such, a :once fixture does not feel like a good fit.

Proposal

I propose adding support for setting :kaocha.plugin.randomize/randomize? on a test suite map and namespace.

Suggested implementation path

See relevant code. If the test-plan has :kaocha.plugin.randomize/randomize? set as a top-level key or in the [:kaocha.testable/meta :kaocha.plugin.randomize/randomize?] path, do not sort the :kaocha.test-plan/tests, but still continue the recursion, allowing the toggle to be on/off at any level.

If the above sounds good, I can provide a PR.

Alternatives

  1. A harsh alternative is to set :kaocha.plugin.randomize/randomize? false directly in tests.edn for all tests that use that config. This is not ideal because most often you want randomize? true.
  2. Always pass --no-randomize to the CLI when running the tests. This is not ideal because it requires the caller to know they must pass that flag for tests to be idempotent.
  3. Execute the expensive operation multiple times. This is, perhaps, the cleanest path, but costs us (potentially significantly) extra time when running tests locally and on CI.
alysbrooks commented 2 years ago

Thank you for your thorough proposal! Two more alternatives to consider:

  1. Memoize the expensive operation. I think this is as clean of a path as your alternative 3.
  2. Use a :once fixture and add a test or tests verifying whatever you need to test about the expensive operation. Arguably this is the best of the alternatives because if your expensive operation fails, you get a relevant test failure, rather than just downstream errors from tests that depend on the expensive operation.

However, I think your proposed PR is still potentially useful. Even if my alternatives work in your case, your solution might be better if there are many expensive operations, if you have a namespace you don't have time to rewrite so it can be randomized yet, or if test randomization isn't worth the implementation costs. In all of those cases, you probably don't want to disable randomization for the sake of a weird namespace. This also provides a migration path for gradually fixing test suites that cannot be randomized yet.

What do you think, @plexus? Randomizing all tests and using :once are definitely what we recommend, so adding code to support something we would usually advise against might not be the best tradeoff. On the other hand, we do try to support a variety of situations with Kaocha. A final advantage for us as consultants is that we could encounter a code base with tests that need to be reworked to support randomization.

kennyjwilli commented 2 years ago

Thanks for the reply @alysbrooks. A fixture does not feel like the right fit in this case because the expensive operation is the one being tested. As a result, if the expensive operation fails (e.g., throws), it would first fail in the fixture, rather than in the actual test. That feels like a mismatch. Certainly something that can be worked around, though.

alysbrooks commented 2 years ago

@kennyjwilli What about option 4, memoization? The downside is that if it does fail, memoization will keep trying the expensive operation. But if the expensive operation tends to fail early on or you have --fail-fast enabled, it should work fine. You could also write a custom memoization function that caches exceptions.

kennyjwilli commented 2 years ago

For sure -- there's definitely ways to workaround it 🙂 Still interested in the feature proposal though.

oxalorg commented 2 years ago

@kennyjwilli I wrote a plugin which de-sorts (or rather natural-sorts based on line number of functions) https://github.com/oxalorg/kaocha-plugin-examples/blob/main/test/hello_test.clj

This is probably a very bad & incomplete solution but I started repl hacking to explore and just ended up with this 🙈

Making this configurable in kaocha itself probably makes more sense!

Also ended up with a find-order function which can probably be helpful if someone ends up working on this feature (again not tested on complex plans): https://github.com/oxalorg/kaocha-plugin-examples/blob/6431cd0987a28ccc9e444ec3da90faf8ed3ed0c5/test/hello_test.clj#L46-L53

plexus commented 2 years ago

Hey @kennyjwilli, nice to see you here :wave:

I think there's merit to the idea, even though it's a fairly niche use case, and relying on ordering is still somewhat of an anti-pattern. But being able to configure things on the testable level, or via a testable's metadata is very kaocha-y. It's a pattern we use elsewhere as well, maximally allow steering Kaocha's behavior through data, which lends more power to plugins and hooks, and opens the door for potentially other use cases as well.

Some details of how I think this would work

In practice, for clojure.test tests, there are three places where it would make sense to configure this key

So you could for instance (case 1)

Or the inverse (case 2)

There's one special case, in that passing --no-randomize should IMO override everything and disable all randomization. That flag exists so people get a repeatable order, so they can check if they are seeing an ordering issue. Currently --no-randomize sets ::randomize? false at the top level config, but per the above that could still be overruled on the suite or namespace level. And I think that makes sense when it's written directly in tests.edn (case 2 above). So maybe we should signal in a different way that randomization is completely disabled via a CLI flag.

This issue did make me realize that we should better document how to swap out core plugins. It's trivial to copy kaocha/plugins/randomize.clj over to your own project, give it a different name, and adjust it to your needs. You can then use

#kaocha/v1
{:plugins ^:replace [...]}

to set it up instead of the default randomize plugin.