Skip to content

Conversation

ikappaki
Copy link
Contributor

Hi,

could you please review patch to have (is (= exp act) evaluate its argument only once when failing. It fixes #712.

Previously, the arguments were evaluated once in the when close, and then once more in the :actual and :expected keys.

I haven't introduced a test case, because this issue manifest itself only when the test fails, which inadvertently will fail the test suite. Thus I was expecting perhaps you could reason about its behavior for the review?
(Though now that I think of it, it might be possible to write a test case for the gen-assert multimethod rather than the is case which will fail the test suite)

Here are the result running the same test in #712 with the fix, it only prints :5 once as expected this time:

basilisp.user=> (require '[basilisp.test :refer [deftest is]])
nil

basilisp.user=> (deftest issue
                    (is (= 8 (println :5))))
#'basilisp.user/issue

basilisp.user=> (issue)
:5
{:failures [{:actual nil :message "Test failure: (= 8 (println :5))" :test-section nil :type :failure :expr (= 8 (println :5)) :expected 8 :test-name "issue" :line 2}]}

Thanks

@chrisrink10 chrisrink10 merged commit 58cff26 into basilisp-lang:main Aug 24, 2023
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.

(basilisp.test/is (= act expr) evaluates its arguments twice on failure
2 participants