Open humorless opened 1 year ago
Patch coverage: 58.97
% and project coverage change: -0.22
:warning:
Comparison is base (
f868e6d
) 75.08% compared to head (c6f7fb2
) 74.86%.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.
Since we might want to make other warnings configurable, I think we should either:
:mute-warnings [:zero-assertions]
.: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.
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.
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.
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 @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 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
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?
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.
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?
This is for #162
kaocha's default behavior will let the tests fail if there are no
is
in assertion.With this MR, we now support a new config/cli args to switch off the
zero-assertions
Example of
tests.edn
This MR can also fix #211
Example of
tests.edn
;; => Results