Allow :kaocha.plugin.randomize/randomize? to be set at any test level

[kennyjwilli]

Fixes #272 by implementing @plexus's proposal here.

Comments and questions on the implementation.

1. I added a macro or-some to the randomize namespace since it removed some repeated nestings. This macro could be moved to the util ns or removed altogether in favor of the nested if-somes.
2. The presence of ::seed also means that the "forced" --no-randomize CLI flag was not set. This could be passed explicitly by adding a new keyword.
3. I don't quite know why straight-sort is called here, but I left it in.
4. I tested this by manually modifying c-tests/foo/hello_test.clj. This was painful since REPL restarts were necessary to detect changes to that file. I assume there's a way around that, but I did not dig. Instead I wrote a unit test for rng-sort. I wasn't sure what the best approach for testing this was, so I left it off the draft version of the PR. Please let me know if there is a preference here. You can find the unit test below.

(deftest rng-sort-test
  (let [rng      ((requiring-resolve 'kaocha.plugin.randomize/rng) 123)
        rng-sort (requiring-resolve 'kaocha.plugin.randomize/rng-sort)]
    (is (= {}
           (rng-sort rng true
                     {})))
    (is (= {:kaocha.test-plan/tests [{:kaocha.plugin.randomize/sort-key -1188957731}]}
           (rng-sort rng true
                     {:kaocha.test-plan/tests [{}]})))
    (is (= {:kaocha.test-plan/tests [{}]}
           (rng-sort rng false
                     {:kaocha.test-plan/tests [{}]})))
    (is (match? {:kaocha.test-plan/tests [{}]}
                (rng-sort rng true
                          {:kaocha.testable/meta   {:kaocha.plugin.randomize/randomize? false}
                           :kaocha.test-plan/tests [{}]})))
    (is (match? {:kaocha.test-plan/tests [{}]}
                (rng-sort rng true
                          {:kaocha.plugin.randomize/randomize? false
                           :kaocha.test-plan/tests [{}]})))))

5. This feature should probably be documented somewhere, but I couldn't find a section about the randomize plugin, so I left that off too. Let me know how you'd like to proceed.
6. command-line.print-config/using---print-config test is failing because the output got reordered due to switching map implementations. It's difficult to make the new output order match the text in print_config.feature since it's in a less optimal order. Suggestions?

[plexus]

Hi @kennyjwilli , thanks for this! I had a first look through the diff. Not very in depth but generally I think it looks good.

I added a macro or-some to the randomize namespace since it removed some repeated nestings. This macro could be moved to the util ns or removed altogether in favor of the nested if-somes.

That's fine, you can leave it there

The presence of ::seed also means that the "forced" --no-randomize CLI flag was not set. This could be passed explicitly by adding a new keyword.

Can you elaborate? Does this mean that passing --no-randomize on the CLI does nothing if there's a fixed ::seed in tests.edn?

I don't quite know why straight-sort is called here, but I left it in.

Good question. Since we first store the ::sort-key and then sort by that I also don't think this should make any difference. We might be able to take that out.

I tested this by manually modifying c-tests/foo/hello_test.clj. This was painful since REPL restarts were necessary to detect changes to that file. I assume there's a way around that, but I did not dig. Instead I wrote a unit test for rng-sort. I wasn't sure what the best approach for testing this was, so I left it off the draft version of the PR. Please let me know if there is a preference here. You can find the unit test below.

Having a unit test like this is definitely useful, so you can go ahead and add them. I guess we could have a .feature test with fixed seeds to get predictable output, which would also give us a place to add docs for randomize. Not sure if you want to put that much effort in, they can be finicky to write.

You can call individual plugin hooks as functions, e.g. (randomize-config-hook ...), (randomize-post-load-hook ...). That's probably a good sweet spot for testing this logic, e.g.

(randomize-post-load-hook
 {:kaocha.test-plan/tests
  [{:kaocha.test-plan/id `a
    :kaocha.testable/meta {::randomize? false}
    :kaocha.test-plan/tests
    [{:kaocha.test-plan/id `a1}
     {:kaocha.test-plan/id `a2}]}
   {:kaocha.test-plan/id `b
    :kaocha.test-plan/tests
    [{:kaocha.test-plan/id `b1}
     {:kaocha.test-plan/id `b2}]}]
  ::randomize? true
  ::seed 1})

[plexus]

This feature should probably be documented somewhere, but I couldn't find a section about the randomize plugin, so I left that off too. Let me know how you'd like to proceed.

Seems we mainly mention the randomization feature in 04_running_kaocha_cli.md

doc/04_running_kaocha_cli.md:Kaocha by default randomizes the order that tests are run: it picks a random
doc/04_running_kaocha_cli.md:Tests should be independent, but this is not always the case. This random order
doc/04_running_kaocha_cli.md:The random seed will be printed at the start of the test run. On the same code
doc/04_running_kaocha_cli.md:Use `--no-randomize` to load suites in the order they are specified, and vars in
doc/04_running_kaocha_cli.md:the order they occur in the source. You can disable randomization in `tests.edn`

and it's mentioned but not elaborated on in 03_configuration.md. I think it might be good to add a dedicated section in 08_plugins, and link to that in 03_configuration.md, "for full details on how to use the randomization feature see the plugin docs".

command-line.print-config/using---print-config test is failing because the output got reordered due to switching map implementations. It's difficult to make the new output order match the text in print_config.feature since it's in a less optimal order. Suggestions?

Right, that's annoying... maybe do something like this? Then the order should matter less.

  Scenario: Using `--print-config`
    When I run `bin/kaocha --print-config`
    Then the output should contain:
      """ clojure
       :kaocha.plugin.randomize/randomize? false,
      """
    And the output should contain:
      """ clojure
       :kaocha/reporter [kaocha.report/dots],
      """
    And the output should contain:
      """ clojure
       :kaocha/color? false,
      """
    And the output should contain:
      """ clojure
       :kaocha/fail-fast? false,
      """
    And the output should contain:
      """ clojure
       :kaocha/tests
       [{:kaocha.testable/type :kaocha.type/clojure.test,
         :kaocha.testable/id :unit,
         :kaocha/ns-patterns ["-test$"],
         :kaocha/source-paths ["src"],
         :kaocha/test-paths ["test"],
         :kaocha.filter/skip-meta [:kaocha/skip]}],
      """

[kennyjwilli]

The presence of ::seed also means that the "forced" --no-randomize CLI flag was not set. This could be passed explicitly by adding a new keyword.
Can you elaborate? Does this mean that passing --no-randomize on the CLI does nothing if there's a fixed ::seed in tests.edn?

Rereading this a few weeks later, I agree that the comment was not clear as all 😅 To answer your question, no, that's not what I meant. Passing --no-randomize on the CLI will always override any other settings. The purpose of my statement was to note the overloaded use of the ::seed key. Its presence indicates if randomization should be used at all, and its value indicates the random seed. Those two use cases could be stored as two different keys if so desired.

Good question. Since we first store the ::sort-key and then sort by that I also don't think this should make any difference. We might be able to take that out.

I went ahead and removed it. 9d5f3ed

Having a unit test like this is definitely useful ...

Added. 5583cda

I think it might be good to add a dedicated section in 08_plugins

Added. LMK if you'd like any revisions, or go ahead and make any edits you'd like. e5f8a55 and 4edf409

Right, that's annoying... maybe do something like this? Then the order should matter less.

Ok, so I updated the test to pass (5cd4553), but, for the life of me, I could not get the final block to pass, so I removed it. I pasted the failing output below, if you have any ideas on a fix. I'd also welcome advise on how to debug this. I see features.steps.kaocha-integration, but I'm not quite sure how that gets called, and if I can easily inject some debuggery from a repl.

The final, failing block.

    And the output should contain:
      """ clojure
       :kaocha/tests
       [{:kaocha.testable/type :kaocha.type/clojure.test,
         :kaocha.testable/id :unit,
         :kaocha/ns-patterns ["-test$"],
         :kaocha/source-paths ["src"],
         :kaocha/test-paths ["test"],
         :kaocha.filter/skip-meta [:kaocha/skip]}],
      """

... and the output.

FAIL in command-line.print-config/using---print-config (test/features/command_line/print_config.feature:30)
the output should contain:
Expected: (substring? needle haystack)
Haystack:
{:kaocha/tests
 [{:kaocha.testable/type :kaocha.type/clojure.test,
   :kaocha.testable/id :unit,
   :kaocha/ns-patterns ["-test$"],
   :kaocha/source-paths ["src"],
   :kaocha/test-paths ["test"],
   :kaocha.filter/skip-meta [:kaocha/skip]}],
 :kaocha/fail-fast? false,
 :kaocha/color? false,
 :kaocha/cli-options {:config-file "tests.edn", :print-config true},
 :kaocha.plugin.randomize/seed 551409332,
 :kaocha.plugin.randomize/randomize? false,
 :kaocha/plugins
 [:kaocha.plugin/randomize
  :kaocha.plugin/filter
  :kaocha.plugin/capture-output],
 :kaocha.plugin.capture-output/capture-output? true,
 :kaocha/reporter [kaocha.report/dots]}

Needle:
 :kaocha/tests
 [{:kaocha.testable/type :kaocha.type/clojure.test,
   :kaocha.testable/id :unit,
   :kaocha/ns-patterns ["-test$"],
   :kaocha/source-paths ["src"],
   :kaocha/test-paths ["test"],
   :kaocha.filter/skip-meta [:kaocha/skip]}],
╭───── Test output ───────────────────────────────────────────────────────
│ /var/folders/t2/bmd89mm95f5cynfdtwx1l11m0000gn/T/kaocha_integration10038483014523596160 $ bin/kaocha --print-config
│ stdout:
│ {:kaocha/tests
│  [{:kaocha.testable/type :kaocha.type/clojure.test,
│    :kaocha.testable/id :unit,
│    :kaocha/ns-patterns ["-test$"],
│    :kaocha/source-paths ["src"],
│    :kaocha/test-paths ["test"],
│    :kaocha.filter/skip-meta [:kaocha/skip]}],
│  :kaocha/fail-fast? false,
│  :kaocha/color? false,
│  :kaocha/cli-options {:config-file "tests.edn", :print-config true},
│  :kaocha.plugin.randomize/seed 551409332,
│  :kaocha.plugin.randomize/randomize? false,
│  :kaocha/plugins
│  [:kaocha.plugin/randomize
│   :kaocha.plugin/filter
│   :kaocha.plugin/capture-output],
│  :kaocha.plugin.capture-output/capture-output? true,
│  :kaocha/reporter [kaocha.report/dots]}
│ 
│ stderr:
│ Downloading: lambdaisland/kaocha-cloverage/maven-metadata.xml from datomic-cloud
│ Downloading: lambdaisland/kaocha-cloverage/maven-metadata.xml from cs-mvn
│ WARNING: Implicit use of clojure.main with options is deprecated, use -M
╰─────────────────────────────────────────────────────────────────────────
1 tests, 5 assertions, 1 failures.

[codecov]

Codecov Report

Attention: Patch coverage is 93.54839% with 2 lines in your changes missing coverage. Please review.

Project coverage is 75.35%. Comparing base (333e2a9) to head (4edf409).
Report is 382 commits behind head on main.

Files	Patch %	Lines
src/kaocha/plugin/randomize.clj	93.54%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #279      +/-   ##
==========================================
- Coverage   75.37%   75.35%   -0.02%     
==========================================
  Files          51       51              
  Lines        2684     2690       +6     
  Branches      248      253       +5     
==========================================
+ Hits         2023     2027       +4     
  Misses        504      504              
- Partials      157      159       +2     

Flag	Coverage Δ
integration	57.39% <87.09%> (+0.05%)	⬆️
unit	69.07% <87.09%> (-0.08%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.