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

Change randomize? at any level #272

Open
kennyjwilli opened this issue Mar 1, 2022 · 6 comments · May be fixed by #279
Open

Change randomize? at any level #272

kennyjwilli opened this issue Mar 1, 2022 · 6 comments · May be fixed by #279
Labels
improvement Incremental improvement of an existing feature

Comments

@kennyjwilli
Copy link

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
Copy link
Member

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
Copy link
Author

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
Copy link
Member

@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
Copy link
Author

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

@oxalorg
Copy link
Member

oxalorg commented Mar 2, 2022

@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
Copy link
Member

plexus commented Mar 3, 2022

Hey @kennyjwilli, nice to see you here 👋

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

  • we check for the :kaocha.plugin.randomize/randomize? at every level of the hierarchy, currently this key is in use but only at the top level
  • in particular we check for it both on the testable itself, and on its metadata. Metadata takes precedence if both are present.
  • each non-leaf test needs to decide if it should randomize its children, for this it considers the value of ::randomize/randomize? of the nearest ancestor where the key is present
  • in other words, setting ::randomize/randomize? to true/false will cause the entire subtree of testables to be randomized/not-randomized, unless any descendant has its own randomize? that overrules it

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

  • top level config
  • test suite config
  • namespace metadata

So you could for instance (case 1)

  • randomize by default (top level :randomize? true, which is the default anyway)
  • exclude a specific test suite :randomize? false on the suite in tests.edn)
  • but randomize a single namespace within that suite (ns ^{:kaocha.plugin.randomize/randomize? true} ...)

Or the inverse (case 2)

  • turn it off at the top level
  • but turn it on for certain tests suites
  • with the exception of certain namespaces

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Incremental improvement of an existing feature
Projects
Status: Candidate
Development

Successfully merging a pull request may close this issue.

4 participants