Wrap executing test to match clojure.test for fixtures #368

NoahTheDuke · 2022-12-08T21:18:52Z

Fixes #367

The fix is to wrap the existing test execution logic in an anonymous function, and use that instead of the given test function itself so that any thrown exceptions are caught by the test wrapping logic and not by an :each fixture.

codecov · 2022-12-08T21:40:58Z

Codecov Report

Base: 75.26% // Head: 75.32% // Increases project coverage by +0.06% 🎉

Coverage data is based on head (0f74774) compared to base (7d6c897).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #368      +/-   ##
==========================================
+ Coverage   75.26%   75.32%   +0.06%     
==========================================
  Files          51       51              
  Lines        2733     2736       +3     
  Branches      256      256              
==========================================
+ Hits         2057     2061       +4     
+ Misses        515      514       -1     
  Partials      161      161

Flag	Coverage Δ
integration	`57.07% <84.21%> (+0.04%)`	⬆️
unit	`69.50% <78.94%> (+0.17%)`	⬆️

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

Impacted Files	Coverage Δ
src/kaocha/type/var.clj	`84.61% <100.00%> (+1.28%)`	⬆️
src/kaocha/type/ns.clj	`97.87% <0.00%> (+2.12%)`	⬆️

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 at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

alysbrooks

Hi, @NoahTheDuke, thanks for the PR! (And thank you for updating the CHANGELOG and test!). I left one suggestion. I think I'll have another person take another look because of how central the changed code is.

alysbrooks · 2022-12-15T00:25:05Z

src/kaocha/type/var.clj

-                                            the-var :kaocha.var/var
-                                            meta'   :kaocha.testable/meta
-                                            :as     testable} test-plan]
+(defn test-var [{test    :kaocha.var/test


I wonder if it would be a little clearer if test-var itself returned a function, rather than having to wrap it later.

Haha if you look at the commits, you'll see i went back and forth on that. I went with wrapping with the function at the call site instead of returning a function because 1) it matches how clojure.test does it and 2) I find it harder to reason about functions that return functions vs letting the wrapping function be part of the implantation details.

However, both are reasonable and I'm good to change it if you desire.

Those reasons make sense. The reason for returning a function is that I'm not sure you'd ever run test-var without wrapping it.

Maybe it just needs a clearer name, like run-test-var. I guess the test in test-var could already be interpreted as a verb, but it's not clear to me.

NoahTheDuke · 2022-12-15T17:39:18Z

Just in case it's not clear, the change is relatively small, it just looks big because of indentation/whitespace changes. If you toggle showing whitespace, you'll see it's "merely" moving the bulk of testable/-run :kaocha.type/var into a separate function and then wrapping that in the original. Not to say you shouldn't examine this thoroughly, just to make my intent clear.

alysbrooks · 2022-12-16T19:39:01Z

Yeah, I tried using difftastic and it reduces the diff considerably. (Although even that tool doesn't point out that most of the remaining changes are really "moves"). My comment about wanting another set of eyes was more me wondering about consequences to this change I didn't think of, as this code path is hit for a large number of tests.

NoahTheDuke · 2023-01-03T17:10:47Z

Hope y'all had a nice holiday break. Is there anything I need to do to get this doubly reviewed?

alysbrooks · 2023-01-06T19:45:26Z

@NoahTheDuke Sorry for the delay. We've been on break this week as well.

There's not anything you need to do to get that review. It might be good to have a more spelled-out process for getting review, but right now we the maintainers contact reviewers internally. Normally, a single review pass is enough; this PR is just a small change I suspect could have subtle implications.

NoahTheDuke · 2023-01-06T20:46:20Z

Cool, thanks @alysbrooks. I don't mind being patient, just want to make sure you're not actually waiting on me for something I missed.

plexus · 2023-01-09T13:59:00Z

I'm having another look at the code, and I think this might interfere with --fail-fast. Not 100% sure. @NoahTheDuke or @alysbrooks would either of you have a moment to verify that fail-fast still works as expected? Thanks!

NoahTheDuke · 2023-01-09T14:49:46Z

By "interfere", do you mean that it would exit kaocha with an exception and stack trace instead of reporting a fail? In both cases, the test run is stopped immediately.

For example, I copied the contents of my fixtures/a-tests/baz/qux_test.clj file into test/unit/koacha/report_test.clj file and ran it with --fail-fast:

$ bin/kaocha --focus kaocha.report-test --fail-fast
--- unit (clojure.test) ---------------------------
kaocha.report-test
  result-failures-test
  dots*-test
  doc-test
  assertion-type-test
  result-test
  fail-fast-test
  dispatch-extra-keys-test
    it dispatches to custom clojure.test/report extensions
    it does nothing if there is no matching multimethod implementation
    it does nothing if it's a key known to Kaocha
    it does nothing if the key is globally marked as "known"
  testing-vars-str-test
    getting info from testable
    explicit file/line override
    clojure.test legacy compatiblity
  print-output-test
  tap-test
  fail-summary-test
  print-expr-test
  doc-print-contexts-test
  nested-test ERROR

Randomized with --seed 1538461704

ERROR in kaocha.report-test/nested-test (report_test.clj:107)
Uncaught exception, not in assertion.
Exception: java.lang.Exception: fake exception
 at kaocha.report_test$fn__10304.invokeStatic (report_test.clj:107)
    kaocha.report_test/fn (report_test.clj:106)
    ...
    clojure.main.main (main.java:40)
14 tests, 43 assertions, 1 errors, 0 failures.

Top 1 slowest kaocha.type/clojure.test (0.12594 seconds, 96.9% of total time)
  unit
    0.12594 seconds average (0.12594 seconds / 1 tests)

Top 1 slowest kaocha.type/ns (0.11073 seconds, 85.2% of total time)
  kaocha.report-test
    0.00738 seconds average (0.11073 seconds / 15 tests)

Top 3 slowest kaocha.type/var (0.05006 seconds, 38.5% of total time)
  kaocha.report-test/result-failures-test
    0.02049 seconds kaocha/report_test.clj:247
  kaocha.report-test/fail-summary-test
    0.01609 seconds kaocha/report_test.clj:180
  kaocha.report-test/tap-test
    0.01348 seconds kaocha/report_test.clj:324

bin/kaocha --fail-fast --focus 'kaocha.report-test/nested-test'

The purpose of my change is to expose these, to not swallow exceptions in test functions. If the "swallowing exceptions" behavior something you want from Kaocha then this change isn't appropriate and I'll just have to maintain my own fork.

Thanks so much for the review.

plexus · 2023-01-10T08:23:45Z

All good then, I just wanted to make sure since fail fast internally uses exceptions.

plexus · 2023-01-10T09:06:28Z

NoahTheDuke · 2023-01-10T13:47:14Z

Ayyyyy thank you!

NoahTheDuke added 2 commits December 8, 2022 16:14

Wrap executing test to match clojure.test for fixtures

ebf54c0

Update changelog

12c4202

NoahTheDuke added 2 commits December 9, 2022 13:02

Move test wrapper to standalone function

a523c8c

Move rest of type/var to standalone function

d59cad8

NoahTheDuke force-pushed the nb/fix-test-wrap branch from 63c6d61 to d59cad8 Compare December 9, 2022 18:16

Update wrapped test var function name

689f86b

alysbrooks reviewed Dec 15, 2022

View reviewed changes

Merge branch 'main' into nb/fix-test-wrap

0f74774

plexus merged commit 596b8f4 into lambdaisland:main Jan 10, 2023

plexus mentioned this pull request Jan 23, 2023

Kaocha crashes if test assertion runs after deftest function #384

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wrap executing test to match clojure.test for fixtures #368

Wrap executing test to match clojure.test for fixtures #368

NoahTheDuke commented Dec 8, 2022

codecov bot commented Dec 8, 2022 •

edited

Loading

alysbrooks left a comment

alysbrooks Dec 15, 2022

NoahTheDuke Dec 15, 2022

alysbrooks Dec 21, 2022

NoahTheDuke commented Dec 15, 2022

alysbrooks commented Dec 16, 2022

NoahTheDuke commented Jan 3, 2023

alysbrooks commented Jan 6, 2023 •

edited

Loading

NoahTheDuke commented Jan 6, 2023

plexus commented Jan 9, 2023

NoahTheDuke commented Jan 9, 2023

plexus commented Jan 10, 2023

plexus commented Jan 10, 2023

NoahTheDuke commented Jan 10, 2023

Wrap executing test to match clojure.test for fixtures #368

Wrap executing test to match clojure.test for fixtures #368

Conversation

NoahTheDuke commented Dec 8, 2022

codecov bot commented Dec 8, 2022 • edited Loading

Codecov Report

alysbrooks left a comment

Choose a reason for hiding this comment

alysbrooks Dec 15, 2022

Choose a reason for hiding this comment

NoahTheDuke Dec 15, 2022

Choose a reason for hiding this comment

alysbrooks Dec 21, 2022

Choose a reason for hiding this comment

NoahTheDuke commented Dec 15, 2022

alysbrooks commented Dec 16, 2022

NoahTheDuke commented Jan 3, 2023

alysbrooks commented Jan 6, 2023 • edited Loading

NoahTheDuke commented Jan 6, 2023

plexus commented Jan 9, 2023

NoahTheDuke commented Jan 9, 2023

plexus commented Jan 10, 2023

plexus commented Jan 10, 2023

NoahTheDuke commented Jan 10, 2023

codecov bot commented Dec 8, 2022 •

edited

Loading

alysbrooks commented Jan 6, 2023 •

edited

Loading