Skip to content

Fix shouldBeEq function#1362

Merged
pakoito merged 9 commits intoarrow-kt:masterfrom
juliankotrba:fix/should-be-eq
Mar 20, 2019
Merged

Fix shouldBeEq function#1362
pakoito merged 9 commits intoarrow-kt:masterfrom
juliankotrba:fix/should-be-eq

Conversation

@juliankotrba
Copy link
Contributor

The passed matcher function of the should function currently only
creates a Result object, but does not do anything with it. To make a
unit test actually fail, a Matcher object should be passed to the
overloaded should function. This commit is changing this behaviour of
the shouldBeEq function.

After applying this fix, the kotlintest test

"shouldBeEq improved" {
      1.shouldBeEq(2, Int.eq())
}

will fail with the following error message:

Expected: 2 but found: 1
java.lang.AssertionError: Expected: 6 but found: 5
at arrow.test.laws.LawKt.shouldBeEqAllNew(Law.kt:31)
...

The passed matcher function of the should function currently only
creates a Result object, but does not do anything with it. To make a
unit test actually fail, a Matcher object should be passed to the
overloaded should function. This commit is changing this behaviour of
the shouldBeEq function.

After applying this fix, the kotlintest test

"shouldBeEq improved" {
      1.shouldBeEq(2, Int.eq())
}

will fail with the following error message:

Expected: 2 but found: 1
java.lang.AssertionError: Expected: 6 but found: 5
	at arrow.test.laws.LawKt.shouldBeEqAllNew(Law.kt:31)
	...
@juliankotrba
Copy link
Contributor Author

juliankotrba commented Mar 13, 2019

Hey @nomisRev, it would be nice if you could take a look at this PR, since you are the original author of shouldBeEq :)
Maybe I misunderstood something..

@pakoito pakoito requested a review from nomisRev March 13, 2019 22:51
@nomisRev
Copy link
Member

@juliankotrba nice catch thanks! 👍

@juliankotrba
Copy link
Contributor Author

@juliankotrba nice catch thanks! 👍

Thanks for taking your time 🙌

@pakoito pakoito merged commit 6ac1940 into arrow-kt:master Mar 20, 2019
@raulraja raulraja mentioned this pull request Sep 10, 2019
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.

4 participants