Allow emptyList to be passed to should[Not]ContainAnyOf, should[Not]BeOneOf and should[Not]BeIn
#4849
Conversation
|
This is going to change the existing behavior of all these matchers right ? |
|
Is there something "official" that indicates what should happen for an empty collection? Like maybe something equivalent in the Kotlin SDK ? |
Kotlin standard library states that:
Mathematically this PR make sense to me: Likewise So I agree with this PR. |
|
Do you think we should add some explicit docs for the matchers that link to why empty collections evaluate to true ? |
if we pivot on this approach, then yes I agree, we should explain it in some detail. because this is a breaking change |
|
Thank you for your response! Since this behavior is mathematically valid and adopted in general libraries, I don't believe it needs to be explained in detail as an exception in the documentation. However, since it is a breaking change, it would be appropriate to increase the major version and announce it as shown in https://kotest.io/docs/changelog.html#breaking-changes-and-removed-deprecated-methods. |
| @@ -31,7 +31,7 @@ infix fun <T> T.shouldBeIn(collection: Collection<T>): T { | |||
| * Assertion to check that this element is not any of [collection]. This assertion checks by value, and not by reference, | |||
| * therefore any instance with same value must not be in [collection], or this will fail. | |||
| * | |||
| * An empty collection will always fail. If you need to check for empty collection, use [Collection.shouldBeEmpty] | |||
| * An empty collection will always pass. | |||
There was a problem hiding this comment.
Can you add a short explanation why "An empty collection will always pass".
| @@ -64,7 +64,7 @@ fun <T> T.shouldBeIn(vararg any: T): T { | |||
| * Assertion to check that this element is not any of [any]. This assertion checks by value, and not by reference, | |||
| * therefore any instance with same value must not be in [any], or this will fail. | |||
| * | |||
| * An empty collection will always fail. If you need to check for empty collection, use [Collection.shouldBeEmpty] | |||
| * An empty collection will always pass. | |||
There was a problem hiding this comment.
Can you add a short explanation why "An empty collection will always pass".
| @@ -99,7 +99,7 @@ infix fun <T> T.shouldBeIn(array: Array<T>): T { | |||
| * Assertion to check that this element is not any of [array]. This assertion checks by value, and not by reference, | |||
| * therefore any instance with same value must not be in [array], or this will fail. | |||
| * | |||
| * An empty array will always fail. If you need to check for empty array, use [Array.shouldBeEmpty] | |||
| * An empty array will always pass. | |||
There was a problem hiding this comment.
Can you add a short explanation why "An empty collection will always pass".
| @@ -29,7 +29,7 @@ infix fun <T> T.shouldBeOneOf(collection: Collection<T>): T { | |||
| * Assertion to check that this instance is not in [collection]. This assertion checks by reference, and not by value, | |||
| * therefore the exact instance must not be in [collection], or this will fail. | |||
| * | |||
| * An empty collection will always fail. If you need to check for empty collection, use [Collection.shouldBeEmpty] | |||
| * An empty collection will always pass. | |||
There was a problem hiding this comment.
Can you add a short explanation why "An empty collection will always pass".
| @@ -61,7 +61,7 @@ fun <T> T.shouldBeOneOf(vararg any: T): T { | |||
| * Assertion to check that this instance is not any of [any]. This assertion checks by reference, and not by value, | |||
| * therefore the exact instance must not be in [any], or this will fail. | |||
| * | |||
| * An empty collection will always fail. If you need to check for empty collection, use [Collection.shouldBeEmpty] | |||
| * An empty collection will always pass. | |||
There was a problem hiding this comment.
Can you add a short explanation why "An empty collection will always pass".
| @@ -34,6 +34,7 @@ kotlin { | |||
| dependencies { | |||
| implementation(projects.kotestProperty) | |||
| implementation(libs.kotlinx.coroutines.core) | |||
| implementation(libs.kotlinx.coroutines.test) | |||
There was a problem hiding this comment.
what did we add this dependency for?
There was a problem hiding this comment.
This is for local testing of :kotest-assertions:kotest-assertions-core.
I would like to develop in a test-driven manner, so the dependency is added first.
There was a problem hiding this comment.
This is for local testing of :kotest-assertions:kotest-assertions-core. I would like to develop in a test-driven manner, so the dependency is added first.
this is an unrelated change. feel free to open a separate PR for that. to keep this PR atomic, please remove the dependency.
other than that, I think the PR is ready to be merged, and is a nice improvement.
The matchers were correct, but the inverted error messages mistakenly swapped the `value` and `anotherInstant` variables, leading to incorrect error messages. I added simple tests asserting the correct messages for the `before` and `after` matchers. I would also suggest to align the wording and punctuation (which I did not do as part of this bug fix - please fix or open an issue in case you agree): - "but it's not" is not consistent with the other formulations and should probably be removed - it is clear from the context (failing assertion) - Some error messages end with a ".", some do not This should probably be merged both into master and 5.9 if a 5.9.2 release should be made. I based the branch on 5.9.1. --------- Co-authored-by: Alex Kuznetsov <Alex.Cue.987@gmail.com>
According the [documentation](https://kotest.io/docs/proptest/property-test-generators-list.html), "`Exhaustive.azstring(1..2)` ... [returns] `a, b, c, ...., yz, zz`". The `yz` seems to imply that it generates all possible permutations; however, this doesn't seem to be the case. For example for `Exhaustive.azstring(2..2)` should produce `aa, ab, ac, ... , zx, zy, zz` but instead produces `aa, bb, cc, ... , xx, yy, zz`. There is a mismatch between the code and the documentation. My changes updates the code to match the written behavior. If the current behavior is intentional, I can change this pull request to update the documentation instead (and update the test to match the current behavior)
20f3b8e to
97d2d65
Compare
…t]BeOneOf` and `should[Not]BeIn` (#4849) <!-- If this PR updates documentation, please update all relevant versions of the docs, see: https://github.com/kotest/kotest/tree/master/documentation/versioned_docs The documentation at https://github.com/kotest/kotest/tree/master/documentation/docs is the documentation for the next minor or major version _TO BE RELEASED_ --> This PR resolves #4847. ## Changes Allow emptyList to be passed as an argument to - `should[Not]ContainAnyOf` - `should[Not]BeOneOf` - `should[Not]BeIn` ### Minor changes - ~Add dependency of libs.kotlinx.coroutines.test for local testing of :kotest-assertions:kotest-assertions-core.~ Moved to #4856 ## Motivation Mathematically, the following assertions should each fail or pass as described: ```kt // Example assertions with empty lists listOf(...).shouldContainAnyOf(emptyList()) // Expected to fail listOf(...).shouldNotContainAnyOf(emptyList()) // Expected to pass listOf(...).shouldBeOneOf(emptyList()) // Expected to fail listOf(...).shouldNotBeOneOf(emptyList()) // Expected to pass listOf(...).shouldBeIn(emptyList()) // Expected to fail listOf(...).shouldNotBeIn(emptyList()) // Expected to pass ``` However, when these assertions are passed an empty collection, an error occurs stating: "Asserting content on empty collection. Use Collection.shouldBeEmpty() instead." These assertions lack consistency with the existing specifications of the following assertions: ```kt listOf(...).shouldContainAll(emptyList()) // Passes listOf(...).shouldNotContainAll(emptyList()) // Fails mapOf(...).shouldContainAll(emptyMap()) // Passes mapOf(...).shouldNotContainAll(emptyMap()) // Fails mapOf(...).shouldContainKeys() // Passes mapOf(...).shouldNotContainKeys() // Fails mapOf(...).shouldContainAnyKeysOf() // Fails mapOf(...).shouldNotContainAnyKeysOf() // Passes mapOf(...).shouldContainValues() // Passes mapOf(...).shouldNotContainValues() // Fails mapOf(...).shouldContainAnyValuesOf() // Fails mapOf(...).shouldNotContainAnyValuesOf() // Passes ``` This PR aims to correct the inconsistent behavior and make the Kotest assertions mathematically accurate and more general. --------- Co-authored-by: Felix Wiemuth <533601+felixwiemuth@users.noreply.github.com> Co-authored-by: Alex Kuznetsov <Alex.Cue.987@gmail.com> Co-authored-by: ej-castillo <eugene.j.castillo@gmail.com>
…kotest-assertions-core (#4856) <!-- If this PR updates documentation, please update all relevant versions of the docs, see: https://github.com/kotest/kotest/tree/master/documentation/versioned_docs The documentation at https://github.com/kotest/kotest/tree/master/documentation/docs is the documentation for the next minor or major version _TO BE RELEASED_ --> ## Changes - Add dependency of libs.kotlinx.coroutines.test to :kotest-assertions:kotest-assertions-core ## Motivation It is necessary to add this dependency to run `:kotest-assertions:kotest-assertions-core` locally. ## Related PRs - #4849
This PR resolves #4847.
Changes
Allow emptyList to be passed as an argument to
should[Not]ContainAnyOfshould[Not]BeOneOfshould[Not]BeInMinor changes
Add dependency of libs.kotlinx.coroutines.test for local testing of :kotest-assertions:kotest-assertions-core.Moved to Add dependency of libs.kotlinx.coroutines.test to :kotest-assertions:kotest-assertions-core #4856Motivation
Mathematically, the following assertions should each fail or pass as described:
However, when these assertions are passed an empty collection, an error occurs stating: "Asserting content on empty collection. Use Collection.shouldBeEmpty() instead."
These assertions lack consistency with the existing specifications of the following assertions:
This PR aims to correct the inconsistent behavior and make the Kotest assertions mathematically accurate and more general.