lambdaisland / kaocha

Full featured next gen Clojure test runner
https://cljdoc.org/d/lambdaisland/kaocha/1.0.861/doc/1-introduction
Eclipse Public License 1.0
796 stars 82 forks source link

Fix :each fixture wrapping #385

Closed NoahTheDuke closed 1 year ago

NoahTheDuke commented 1 year ago

Moving the type/report-count portion of testable/-run :var into test-var wrapped it in the join-fixtures calls, meaning that the is assertion was in the tail position and returned the result (a boolean) instead of returning the merged testable object.

To fix, I've pulled the type/report-count portion back into testable/-run :var and have added a top-level test that fail without this change.

Closes #384

NoahTheDuke commented 1 year ago

The description is slightly more specific than necessary. The result of the tail call of test is returned, not just assertions.

codecov[bot] commented 1 year ago

Codecov Report

Base: 75.74% // Head: 75.54% // Decreases project coverage by -0.21% :warning:

Coverage data is based on head (4621cca) compared to base (8e9cf5f). Patch coverage: 74.28% of modified lines in pull request are covered.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #385 +/- ## ========================================== - Coverage 75.74% 75.54% -0.21% ========================================== Files 51 51 Lines 2758 2772 +14 Branches 259 260 +1 ========================================== + Hits 2089 2094 +5 - Misses 512 519 +7 - Partials 157 159 +2 ``` | Flag | Coverage Δ | | |---|---|---| | integration | `56.63% <37.14%> (-0.22%)` | :arrow_down: | | unit | `69.69% <62.85%> (-0.18%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#carryforward-flags-in-the-pull-request-comment) to find out more. | [Impacted Files](https://codecov.io/gh/lambdaisland/kaocha/pull/385?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | Coverage Δ | | |---|---|---| | [src/kaocha/watch.clj](https://codecov.io/gh/lambdaisland/kaocha/pull/385?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-c3JjL2thb2NoYS93YXRjaC5jbGo=) | `71.67% <60.86%> (-2.30%)` | :arrow_down: | | [src/kaocha/type/var.clj](https://codecov.io/gh/lambdaisland/kaocha/pull/385?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-c3JjL2thb2NoYS90eXBlL3Zhci5jbGo=) | `84.61% <100.00%> (ø)` | | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None)

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

alysbrooks commented 1 year ago

I would have similar concerns as to the original PR (changing a fundamental part of the code that actually runs tests), but this partly reverts a previous change and we have tests for both.

As I've thought about it more, I do wonder whether assertions in fixtures is really the best approach. Still, I don't think the inadvisability is a good reason to break compatibility, and I think the behavior this PR implements (returning the merged testable) makes more sense anyway.

NoahTheDuke commented 1 year ago

Thanks so much. Sorry for the hassle. A real clear cut case of being too clever with my refactoring instead of touching the minimal lines to affect the desired change.

plexus commented 1 year ago

Released in v1.76.1230

[lambdaisland/kaocha "1.76.1230"]                 ;; deps.edn
{lambdaisland/kaocha {:mvn/version "1.76.1230"}}  ;; project.clj