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.

Sign up for GitHub

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

Jump to bottom

Increase test coverage for :fail-fast? #85

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Increase test coverage for :fail-fast? #85

wants to merge 1 commit into from

Conversation

vemv
Copy link

@vemv vemv commented Oct 7, 2021

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

atomist bot commented Oct 7, 2021

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

vemv commented Oct 7, 2021

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:

image

@vemv vemv closed this Oct 7, 2021
@vemv vemv reopened this Oct 7, 2021
@vemv
Copy link
Author

vemv commented Oct 7, 2021

(Fat-fingered the close button)

weavejester
weavejester requested changes Oct 8, 2021
View reviewed changes
Copy link
Owner

@weavejester weavejester left a comment

Choose a reason for hiding this comment

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

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

eftest/test/eftest/runner_test.clj Outdated
Comment on lines 5 to 8
(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))
Copy link
Owner

Choose a reason for hiding this comment

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

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.

eftest/test/eftest/runner_test.clj Outdated
(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)
Copy link
Owner

Choose a reason for hiding this comment

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

Line length should be 80 characters at most.

eftest/test/eftest/runner_test.clj Outdated
(clojure.test/deftest throwing-test
(clojure.test/is (= 1 1)))
Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

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

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

Copy link
Owner

Choose a reason for hiding this comment

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

Additional whitespace.

eftest/test/eftest/runner_test.clj Outdated
Comment on lines 10 to 11
;; Example test namespaces may have a prefix such as `ns-0`
;; so that they can be deterministically sorted.
Copy link
Owner

Choose a reason for hiding this comment

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

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

@vemv
Copy link
Author

vemv commented Oct 9, 2021

Thanks for the review! It's ready again.

weavejester
weavejester requested changes Oct 10, 2021
View reviewed changes
Copy link
Owner

@weavejester weavejester left a comment

Choose a reason for hiding this comment

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

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

eftest/test/eftest/runner_test.clj Outdated
(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 [])
Copy link
Owner

Choose a reason for hiding this comment

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

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.

eftest/test/eftest/runner_test.clj Outdated
(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)
Copy link
Owner

Choose a reason for hiding this comment

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

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

eftest/test/eftest/runner_test.clj Outdated
Comment on lines 24 to 26
(swap! eftest.runner-test/tests-completed
conj
:throwing-test-in-fixtures.side-effect)
Copy link
Owner

Choose a reason for hiding this comment

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

Perhaps:

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

@vemv
Copy link
Author

vemv commented Oct 13, 2021

Ready again!

@vemv
Copy link
Author

vemv commented Oct 19, 2021

(ping)

@weavejester
Copy link
Owner

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

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

atomist bot commented Oct 19, 2021

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
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 vemv force-pushed the increase-coverage-for-fail-fast branch from 7cb3632 to 9f31168 Compare October 22, 2021 09:14
@atomist
Copy link

atomist bot commented Oct 22, 2021

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

vemv commented Oct 22, 2021

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

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@weavejester weavejester weavejester requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@vemv @weavejester