kaocha icon indicating copy to clipboard operation
kaocha copied to clipboard

add config for zero assertions switcher in config and cli args

Open humorless opened this issue 2 years ago • 10 comments

This is for #162

kaocha's default behavior will let the tests fail if there are no is in assertion.

laurencechen qqq $ ./bin/kaocha  
[(F)]
Randomized with --seed 1954406926

FAIL in qqq-test/example-test (qqq_test.clj:5)
Test ran without assertions. Did you forget an (is ...)?
1 tests, 1 assertions, 1 failures.

With this MR, we now support a new config/cli args to switch off the zero-assertions

Example of tests.edn

#kaocha/v1
{:tests [{:id          :unit
          :test-paths  ["test" "src"]
          :ns-patterns [".*"]}]
:warnings {:zero-assertions :silent}
}

This MR can also fix #211

Example of tests.edn

#kaocha/v1
{:tests [{:id          :unit
          :test-paths  ["test" "src"]
          :ns-patterns [".*"]}]
:warnings {:zero-tests :error}
}

;; => Results 截圖 2023-04-21 下午4 55 36

humorless avatar Mar 16 '23 16:03 humorless

Codecov Report

Patch coverage: 58.97% and project coverage change: -0.22 :warning:

Comparison is base (f868e6d) 75.08% compared to head (c6f7fb2) 74.86%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #404      +/-   ##
==========================================
- Coverage   75.08%   74.86%   -0.22%     
==========================================
  Files          52       52              
  Lines        2797     2809      +12     
  Branches      267      270       +3     
==========================================
+ Hits         2100     2103       +3     
- Misses        530      536       +6     
- Partials      167      170       +3     
Flag Coverage Δ
integration 56.24% <17.94%> (-0.17%) :arrow_down:
unit 69.20% <58.97%> (-0.19%) :arrow_down:

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

Impacted Files Coverage Δ
src/kaocha/report.clj 83.79% <ø> (ø)
src/kaocha/specs.clj 82.43% <38.46%> (-1.13%) :arrow_down:
src/kaocha/api.clj 71.02% <40.00%> (-3.72%) :arrow_down:
src/kaocha/hierarchy.clj 96.72% <50.00%> (-1.59%) :arrow_down:
src/kaocha/config.clj 91.01% <90.90%> (-0.52%) :arrow_down:
src/kaocha/runner.clj 45.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

codecov[bot] avatar Mar 16 '23 16:03 codecov[bot]

Since we might want to make other warnings configurable, I think we should either:

  • Accept a list of warnings to mute like :mute-warnings [:zero-assertions].
  • Allow for both increasing the warning level (to a failing error) and muting it: :warnings {:zero-assertions :silent :unwrapped-values-is :error}. (:unwrappd-values-is is a hypothetical other warning we could cover.)

I didn't closely review the rest of the code (although it looks good on first glance) since I think it will have to change if you take one of my suggestions.

alysbrooks avatar Mar 20 '23 17:03 alysbrooks

#162

@plexus Arne, can you also provide your ideas on my change? Alys provided me a configuration option/ arguments design seems more elegant, but I am not sure should we do it now.

humorless avatar Mar 22 '23 08:03 humorless

I agree with Alys, we have a number of warnings that could be configurable. I would rather not introduce a new top level key for each. Instead we should introduce a pattern that people can easily extend when they want to make other warnings configurable.

plexus avatar Mar 22 '23 16:03 plexus

I'm leaning toward the latter option (the :warnings map), for what it's worth. That would let us support warnings that are turned off by default (perhaps because they're alpha) and allow for a stricter configuration on CI.

alysbrooks avatar Mar 31 '23 19:03 alysbrooks

@alysbrooks @plexus

I have implemented the config argument as you suggested. On the other hand, in this case, do I still need to implement warnings in command-line-arguments? And if so, how to do it? Any suggestions?

humorless avatar Apr 05 '23 04:04 humorless

@humorless Yes, that's tricky. I'm not sure there's a great way to do key-value command line arguments. (I found some discussion here). Maybe we could use an approach similar to -Jkey=value like Clojure CLI? (I guess we could use W for warning? Or E for error?) Although maybe --warning name=level would be better?

Usually I'd expect people would modify it in tests.edn, but I suppose you could have a situation where you want to disable it on the command line because you're getting false positives as you work on a test? Perhaps we could save the flag for a later PR?

alysbrooks avatar Apr 07 '23 23:04 alysbrooks

@alysbrooks

I also tried to provide a solution to https://github.com/lambdaisland/kaocha/issues/211 in this MR. Now, I allow

:warnings {:zero-tests :error}

or

:warnings {:zero-tests :silent}

in tests.edn

By the way, I somewhat prefer to delay the command line argument design for later PR. I really think that is there really a need to control kaocha to this level with command line arguments?

humorless avatar Apr 21 '23 09:04 humorless

By the way, I somewhat prefer to delay the command line argument design for later PR. I really think that is there really a need to control kaocha to this level with command line arguments?

Yes, that makes sense to me.

alysbrooks avatar May 06 '23 01:05 alysbrooks

A possible refactoring would be a function that outputs using the correct level based on the warning. For example, (notify warnings :some-key "message") so we don't have to repeat the (if ((:some-warning warnings) :warning) ,,,) code multiple times. But maybe we should first look at the rest of the code base?

alysbrooks avatar May 06 '23 01:05 alysbrooks