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

fix: display mismatch reasons in "following elements were mismatched" #1362

Closed
wants to merge 1 commit into from
Closed

fix: display mismatch reasons in "following elements were mismatched" #1362

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions ...est/kotlin/ch/tutteli/atrium/api/fluent/en_GB/samples/ThrowableFeatureExtractorSamples.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package ch.tutteli.atrium.api.fluent.en_GB.samples

import ch.tutteli.atrium.api.fluent.en_GB.*
import ch.tutteli.atrium.api.verbs.internal.expect
import ch.tutteli.atrium.specs.integration.IterableToContainSpecBase.Companion.toContainSize
import kotlin.test.Test

class ThrowableFeatureExtractorSamples {
Expand All @@ -15,6 +16,33 @@ class ThrowableFeatureExtractorSamples {
}
}

@Test
fun messageFeature2() {
data class User(val login: String, val password: String)

expect(listOf(User("joe", "qwerty"), User("j", "qwerty1"))) {
toHaveElementsAndAll {
feature("login", { login })
.feature("size", { length })
.because("login should be longer than one letter") {
toBeGreaterThan(1)
}
feature("password", { password })
.because("password should be secure") {
notToEqual("qwerty")
}
}
}

expect(listOf(Throwable("abc"))) {
toHaveElementsAndAll {
message
.toContain("d")
.toStartWith("z")
}
}
}

@Test
fun messageFeature() {
expect(RuntimeException("abc"))
Expand Down
3 changes: 1 addition & 2 deletions atrium-core/src/commonMain/kotlin/ch/tutteli/atrium/creating/impl/BaseExpectImpl.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,7 @@ abstract class BaseExpectImpl<T>(
representationInsteadOfFeature?.let { provider ->
maybeSubject.fold({ null }) { provider(it) }
} ?: maybeSubject.getOrElse {
// a RootExpect without a defined subject is almost certain a bug
Text(SHOULD_NOT_BE_SHOWN_TO_THE_USER_BUG)
Text.EMPTY
Copy link
Owner

Choose a reason for hiding this comment

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

Please don't change this in the end, if you saw this text in reporting, then it is most likely an implementation bug 😉

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well, what is your recommendation then?
Currently, Atrium uses an explicit None subject in order to render elements need all: block.:

atrium/logic/atrium-logic/src/commonMain/kotlin/ch/tutteli/atrium/logic/impl/containsHelpers.kt

Line 45 in 65227ad

it.collectAssertions(container, None, assertionCreatorOrNull)

}
}
}
2 changes: 1 addition & 1 deletion ...onMain/kotlin/ch/tutteli/atrium/reporting/text/impl/TextFeatureAssertionGroupFormatter.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,6 @@ class TextFeatureAssertionGroupFormatter(
parameterObject: AssertionFormatterParameterObject
) : AssertionGroup by assertionGroup {
override val description: Translatable = newName
override val representation: Any = if(parameterObject.isNotInExplanatoryFilterGroup()) assertionGroup.representation else Text.EMPTY
override val representation: Any = assertionGroup.representation
Copy link
Owner

Choose a reason for hiding this comment

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

please revert in the end

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What would be your suggestion then?

Apparently, the current implementation in Atrium is "whatever is displayed in explanatory goes WITHOUT representation".
At the same time, the only way to put "warning sign" is to wrap it in explanatory.

That creates a deadlock, and I think it is wrong that "explanatory" implies "remove all representations". I just do not understand why it removes the representations. I believe the ones who want to display message "without representation" should call the relevant APIs "without representation", and it should not be "explanatory" that hides representations.

Copy link
Owner

@robstoll robstoll Mar 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think it is wrong that "explanatory" implies "remove all representations"

The reason why we remove the representation is because usually we explain what expectation was defined without it being bound to a subject. For instance, the ◆ elements need all: part. The expectations shown there are not bound to a specific element in the Iterable.

At the same time, the only way to put "warning sign" is to wrap it in explanatory.

I see now better where your request for a non-fluent assertionBuilder comes from :)

I see overlapping topics here. I think one way forward would be that we can define that a certain Assertion should be shown as is within an explanatory group, i.e. kind of escape the explanatory mechanism. This way we would also only show failing expectations etc. as it was not within an explanatory group.

Another approach would be that we allow more freely what bullet points are used and then you would not use an explanatory group in your case but just a regular list group with another bullet point.

Have to go, wil, think about it later on.

Copy link
Owner

@robstoll robstoll Mar 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

We need to take the approach with escaping because otherwise we loose context. for this to work, we need:

  • change from NonFiltering count to FilteringStack
  • A new GroupAssertionType EscapeNonFiltering which behaves like an invisible group but additionally modifies the stack

I can give further pointers in case you want to tackle this. I am writing on top of my head, would need to take a closer look regarding the exact places and names.

}
}
23 changes: 21 additions & 2 deletions ...logic/src/commonMain/kotlin/ch/tutteli/atrium/logic/impl/DefaultIterableLikeAssertions.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import ch.tutteli.atrium.assertions.Assertion
import ch.tutteli.atrium.assertions.AssertionGroup
import ch.tutteli.atrium.assertions.builders.*
import ch.tutteli.atrium.core.Option
import ch.tutteli.atrium.core.Some
import ch.tutteli.atrium.core.getOrElse
import ch.tutteli.atrium.creating.AssertionContainer
import ch.tutteli.atrium.creating.Expect
Expand All @@ -20,8 +21,10 @@ import ch.tutteli.atrium.logic.creating.iterable.contains.steps.impl.EntryPointS
import ch.tutteli.atrium.logic.creating.iterable.contains.steps.notCheckerStep
import ch.tutteli.atrium.logic.creating.transformers.FeatureExtractorBuilder
import ch.tutteli.atrium.logic.creating.typeutils.IterableLike
import ch.tutteli.atrium.reporting.Text
import ch.tutteli.atrium.reporting.translating.Translatable
import ch.tutteli.atrium.reporting.translating.TranslatableWithArgs
import ch.tutteli.atrium.translations.DescriptionBasic
import ch.tutteli.atrium.translations.DescriptionBasic.NOT_TO_HAVE
import ch.tutteli.atrium.translations.DescriptionBasic.TO_HAVE
import ch.tutteli.atrium.translations.DescriptionIterableLikeExpectation.*
Expand Down Expand Up @@ -135,9 +138,25 @@ class DefaultIterableLikeAssertions : IterableLikeAssertions {

val assertions = ArrayList<Assertion>(2)
assertions.add(createExplanatoryAssertionGroup(container, assertionCreatorOrNull))
val mismatches = createIndexAssertions(list) { (_, element) ->
mismatchIf(allCreatedAssertionsHold(container, element, assertionCreatorOrNull))
val mismatches = list.mapIndexedNotNull { index, e ->
val assertion = if (assertionCreatorOrNull != null) {
container.collectBasedOnSubject(Some(e as E), assertionCreatorOrNull)
} else {
// TODO: assertionCreatorOrNull should be non-nullable, and this "to be null" should be a part of expectation
assertionBuilder.createDescriptive(DescriptionBasic.TO_BE, Text.NULL) {
e == null
}
Comment on lines +145 to +148
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is strange to handle e == null here. I wish assertionCreatorOrNull was non-nullable.

Copy link
Owner

@robstoll robstoll Mar 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I agree, this functionality could be refactored. Is due to some legacy behaviour introduced in Atrium 0.2.0 or the like and wouldn't be necessary any more. Going to create a discussion about it

}

assertion.takeIf { mismatchIf(it.holds()) }?.let {
assertionBuilder
.feature
.withDescriptionAndRepresentation(TranslatableWithArgs(INDEX, index), e)
.withAssertion(it)
.build()
}
Comment on lines +141 to +157
Copy link
Owner

@robstoll robstoll Mar 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

You should do the modification within createIndexAssertions and not here - I suggest you rename createIndexAssertions to createMismatch as it is used only for this purpose

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Of course, it should replace the current createIndexAssertions, however, I tried to implement something minimal to see what are other changes required.

One of the takeaways, for now, is the current implementation shows both successful and failing assertions, and it does not distinguish between successful and failing ones.
For instance login: "joe" was successful, however, it was displayed the same as password: "qwerty" which failed.

}

if (mismatches.isNotEmpty()) assertions.add(createExplanatoryGroupForMismatches(mismatches))

decorateAssertion(
Expand Down
12 changes: 9 additions & 3 deletions misc/atrium-specs/src/commonMain/kotlin/ch/tutteli/atrium/specs/AssertionCreatorSpec.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import ch.tutteli.atrium.api.fluent.en_GB.*
import ch.tutteli.atrium.api.verbs.internal.expect
import ch.tutteli.atrium.creating.Expect
import ch.tutteli.atrium.creating.ErrorMessages
import ch.tutteli.atrium.logic._logic
import org.spekframework.spek2.Spek
import org.spekframework.spek2.style.specification.describe

Expand All @@ -20,9 +21,14 @@ abstract class AssertionCreatorSpec<T>(
describe("fun `$name`") {
it("throws and only reports the lambda which was empty") {
expect {
expect(subject)
.createAssertionOk()
.createAssertionFail()
try {
expect(subject)
.createAssertionOk()
.createAssertionFail()
}catch(it: Error){
println("err: ${it.message}, ${it.cause}")
throw it
}
}.toThrow<AssertionError> {
message {
toContain(
Expand Down