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

add config for zero assertions switcher in config and cli args #404

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

humorless
Copy link
Member

@humorless humorless commented Mar 16, 2023

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 humorless requested a review from alysbrooks March 16, 2023 16:42
@codecov
Copy link

codecov bot commented Mar 16, 2023

Codecov Report

Patch coverage: 58.97% and project coverage change: -0.22 ⚠️

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%) ⬇️
unit 69.20% <58.97%> (-0.19%) ⬇️

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%) ⬇️
src/kaocha/api.clj 71.02% <40.00%> (-3.72%) ⬇️
src/kaocha/hierarchy.clj 96.72% <50.00%> (-1.59%) ⬇️
src/kaocha/config.clj 91.01% <90.90%> (-0.52%) ⬇️
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.

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

@@ -8,6 +8,11 @@
[tag parent]
(alter-var-root #'hierarchy derive tag parent))

(defn underive!
"Add a parent/child relationship to kaocha's keyword hierarchy."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This removes a parent/child relationship, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then I'd suggest:

Suggested change
"Add a parent/child relationship to kaocha's keyword hierarchy."
"Remove a parent/child relationship from Kaocha's keyword hierarchy."

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you planning to make this change?

@alysbrooks
Copy link
Member

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.

@humorless
Copy link
Member Author

#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.

@plexus
Copy link
Member

plexus commented Mar 22, 2023

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.

@alysbrooks
Copy link
Member

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.

@humorless humorless force-pushed the laurence/add-config-for-zero-assertions-switch branch from 42db155 to 7ecc8dd Compare April 5, 2023 04:13
@humorless humorless force-pushed the laurence/add-config-for-zero-assertions-switch branch from 7ecc8dd to c178ed0 Compare April 5, 2023 04:16
@humorless
Copy link
Member Author

@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?

@alysbrooks
Copy link
Member

@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?

@humorless
Copy link
Member Author

@alysbrooks

I also tried to provide a solution to #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?

@alysbrooks
Copy link
Member

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

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Interpret the "0 test" situation as a failure
3 participants