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.

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

Tests: MonadCombine->Alternative, add missing ones #2037

Conversation

vendethiel
Copy link
Contributor

Wrt guard test... This one doesn't seem amazing, but I wasn't too sure what else to test.

@codecov-io
Copy link

codecov-io commented Nov 23, 2017

Codecov Report

Merging #2037 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #2037   +/-   ##
======================================
  Coverage      95%     95%           
======================================
  Files         311     311           
  Lines        5266    5266           
  Branches      131     131           
======================================
  Hits         5003    5003           
  Misses        263     263

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0813246...9a7024a. Read the comment docs.

@vendethiel
Copy link
Contributor Author

I guess they're technically went through here.

@@ -2,6 +2,14 @@ package cats
package tests

class MonadCombineSuite extends CatsSuite {
Copy link
Member

Choose a reason for hiding this comment

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

How about renaming the class as well? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, nice catch!


test("guard") {
forAll { (b: Boolean) =>
Alternative[Option].guard(b).isDefined should === (b)
Copy link
Member

Choose a reason for hiding this comment

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

This test case will probably run 100 times, but only two values are actually tested. I'd rather generate different valid Alternative instances.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure how to do that with forall. Do you have an example testcase I can look into and copy the behavior of?

Copy link
Contributor

Choose a reason for hiding this comment

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

You don’t need forall. Just write a def for the law, then call it for two inputs.

Copy link
Contributor

@johnynek johnynek 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 improved tests.


test("guard") {
forAll { (b: Boolean) =>
Alternative[Option].guard(b).isDefined should === (b)
Copy link
Contributor

Choose a reason for hiding this comment

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

You don’t need forall. Just write a def for the law, then call it for two inputs.

@vendethiel vendethiel force-pushed the move-monadcombine-tests-to-alternative-and-complete branch from 3a4734e to 9a7024a Compare November 28, 2017 11:13
@vendethiel
Copy link
Contributor Author

Pushed this version. I can add more tests that work the way @LukaJCB wanted if someone can give me some pointers :).

Copy link
Member

@LukaJCB LukaJCB left a comment

Choose a reason for hiding this comment

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

Thanks!

@kailuowang kailuowang merged commit 7f403ef into typelevel:master Nov 28, 2017
@vendethiel vendethiel deleted the move-monadcombine-tests-to-alternative-and-complete branch November 28, 2017 14:21
@vendethiel
Copy link
Contributor Author

Thanks!

@kailuowang kailuowang added this to the 1.0.0 milestone Dec 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants