Increase test coverage for `:fail-fast`? by vemv · Pull Request #85 · weavejester/eftest

vemv · 2021-10-07T11:40:42Z

This will help reliably coming up with a fix for #75.

Coverage is extended by:

providing counterexamples for each test case
also exercising exception handling, whether it happens in the deftest body, or in fixtures.

atomist · 2021-10-07T11:41:23Z

Commit messages of this repository should follow the seven rules of a great Git commit message, as mentioned in the project's contributing guidelines. It looks like there's a few issues with the commit messages in this pull request:

The commit message should not contain markdown.
The commit message body has a line over 72 characters.

vemv · 2021-10-07T11:43:33Z

The commit message should not contain markdown.

Are you on board with this one? Some months ago GH added pretty rendering for markdown in various places of the UI, which invites one to create less ambiguous communication:

vemv · 2021-10-07T11:45:17Z

(Fat-fingered the close button)

weavejester

Thanks for the patch! Some changes required as part of the review.

weavejester · 2021-10-08T14:11:38Z

eftest/test/eftest/runner_test.clj

+(def proof-init-state [])
+(def proof
+  "Helps proving that a given deftest was run (and in a specific order, relative to other deftests)."
+  (atom proof-init-state))


This could be changed to:

(def tests-completed (atom []))

The tests-completed name describes what it holds, and the fact it's a vector indicates it's ordered.

weavejester · 2021-10-08T14:11:55Z

eftest/test/eftest/runner_test.clj

+(clojure.core/refer-clojure)
+(clojure.core/require 'clojure.test)
+(clojure.test/use-fixtures :once (fn [t]
+                                   (swap! eftest.runner-test/proof conj :throwing-test-in-fixtures.side-effect)


Line length should be 80 characters at most.

weavejester · 2021-10-08T14:12:50Z

eftest/test/eftest/runner_test.clj

+(clojure.test/deftest throwing-test
+  (clojure.test/is (= 1 1)))


Why is this a throwing-test when it doesn't throw?

Why is this a throwing-test when it doesn't throw?

It 'throws' via the fixture but either way, I renamed it

weavejester · 2021-10-08T14:13:10Z

eftest/test/eftest/runner_test.clj

+        (is (= {:test 2 :fail 2 :error 0} result))
+        (is (= [:single-failing-test.side-effect :another-failing-test.side-effect]
+               @proof))))
+


Additional whitespace.

weavejester · 2021-10-08T14:14:52Z

eftest/test/eftest/runner_test.clj

+;; Example test namespaces may have a prefix such as `ns-0`
+;; so that they can be deterministically sorted.


Wrap at 80 characters, remove markdown backtick, remove "may".

vemv · 2021-10-09T16:36:30Z

Thanks for the review! It's ready again.

weavejester

Thanks for the changes. There's a couple more, but overall this is looking fine.

weavejester · 2021-10-10T12:29:07Z

eftest/test/eftest/runner_test.clj

-(def proof
-  "Helps proving that a given deftest was run (and in a specific order, relative to other deftests)."
-  (atom proof-init-state))
+(def tests-completed-init-state [])


This can be removed. Just use (atom []). By using reset or atom, you know whatever's there is the initial state. Using a var makes it less readable because you have to look up what the initial state actually is.

weavejester · 2021-10-10T12:32:49Z

eftest/test/eftest/runner_test.clj

+(clojure.core/refer-clojure)
+(clojure.core/require 'clojure.test)
+(clojure.test/deftest throwing-test
+  (swap! eftest.runner-test/tests-completed conj :throwing-test.side-effect)


The .side-effect can be removed from the keywords. It doesn't add any more necessary information.

weavejester · 2021-10-10T12:33:50Z

eftest/test/eftest/runner_test.clj

+    (swap! eftest.runner-test/tests-completed
+           conj
+           :throwing-test-in-fixtures.side-effect)


Perhaps:

(swap! eftest.runner-test/tests-completed conj :throwing-test-in-fixtures)

vemv · 2021-10-13T08:31:06Z

Ready again!

vemv · 2021-10-19T21:30:09Z

(ping)

weavejester · 2021-10-19T21:54:41Z

Sorry for the delay. It looks fine - can you squash down your commits?

atomist · 2021-10-19T22:01:13Z

Commit messages of this repository should follow the seven rules of a great Git commit message, as mentioned in the project's contributing guidelines. It looks like there's a few issues with the commit messages in this pull request:

The commit message should not contain markdown.
The commit message body has a line over 72 characters.

This will help reliably coming up with a fix for weavejester#75. Coverage is extended by: * providing counterexamples for each test case * also exercising exception handling, whether it happens in the deftest body, or in fixtures.

atomist · 2021-10-22T09:14:18Z

Commit messages of this repository should follow the seven rules of a great Git commit message, as mentioned in the project's contributing guidelines. It looks like there's a few issues with the commit messages in this pull request:

The commit message should not contain markdown.

vemv · 2021-10-22T09:14:58Z

The commit message body has a line over 72 characters.

Satisfied

The commit message should not contain markdown.

Left as-is see #85 (comment)

weavejester · 2021-10-22T12:22:19Z

The commit message should not contain markdown.

Are you on board with this one? Some months ago GH added pretty rendering for markdown in various places of the UI, which invites one to create less ambiguous communication:

I'd prefer plaintext, please. Not all git log viewers support markdown. So a better commit message might be:

Increase test coverage for :fail-fast option

Coverage is extended by checking exceptions and the base case when
:fail-fast is false.

You also mention #75 in the commit message, but these tests pass, don't they? How do you see them helping with that issue?

vemv closed this Oct 7, 2021

vemv reopened this Oct 7, 2021

weavejester requested changes Oct 8, 2021

View reviewed changes

weavejester requested changes Oct 10, 2021

View reviewed changes

vemv force-pushed the increase-coverage-for-fail-fast branch from 22fb442 to 7cb3632 Compare October 19, 2021 22:01

Increase test coverage for :fail-fast?

9f31168

This will help reliably coming up with a fix for weavejester#75. Coverage is extended by: * providing counterexamples for each test case * also exercising exception handling, whether it happens in the deftest body, or in fixtures.

vemv force-pushed the increase-coverage-for-fail-fast branch from 7cb3632 to 9f31168 Compare October 22, 2021 09:14

		(clojure.test/deftest throwing-test
		(clojure.test/is (= 1 1)))

		;; Example test namespaces may have a prefix such as `ns-0`
		;; so that they can be deterministically sorted.

Conversation

vemv commented Oct 7, 2021

Uh oh!

atomist bot commented Oct 7, 2021

Uh oh!

vemv commented Oct 7, 2021

Uh oh!

vemv commented Oct 7, 2021

Uh oh!

weavejester left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vemv commented Oct 9, 2021

Uh oh!

weavejester left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vemv commented Oct 13, 2021

Uh oh!

vemv commented Oct 19, 2021

Uh oh!

weavejester commented Oct 19, 2021

Uh oh!

atomist bot commented Oct 19, 2021

Uh oh!

atomist bot commented Oct 22, 2021

Uh oh!

vemv commented Oct 22, 2021

Uh oh!

weavejester commented Oct 22, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants