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

vlsi · 2023-03-18T10:01:06Z

I confirm that I have read the Contributor Agreements v1.0, agree to be bound on them and confirm that my contribution is compliant.

Fixes robstoll#1359

vlsi · 2023-03-18T10:02:00Z

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

+ // 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
+ }


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

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

vlsi · 2023-03-18T10:03:37Z

The fix makes

        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")
                    }
            }
        }

to produce

I expected subject: [User(login=joe, password=qwerty), User(login=j, password=qwerty1)]        (java.util.Arrays.ArrayList <1937662030>)
◆ elements need all: 
    » ▶ login: 
        ◾ ▶ size: 
            ◾ to be greater than: 1        (kotlin.Int <987792975>)
            ℹ because: login should be longer than one letter
    » ▶ password: 
        ◾ not to equal: "qwerty"        <952055826>
        ℹ because: password should be secure
    ❗❗ following elements were mismatched: 
       ⚬ ▶ index 0: User(login=joe, password=qwerty)        (ch.tutteli.atrium.api.fluent.en_GB.samples.ThrowableFeatureExtractorSamples$messageFeature2$User <1635068416>)
           ◾ ▶ login: "joe"        <1130906588>
               ◾ ▶ size: 3        (kotlin.Int <439323450>)
                   ◾ to be greater than: 1        (kotlin.Int <987792975>)
                   ℹ because: login should be longer than one letter
           ◾ ▶ password: "qwerty"        <952055826>
               ◾ not to equal: "qwerty"        <952055826>
               ℹ because: password should be secure
       ⚬ ▶ index 1: User(login=j, password=qwerty1)        (ch.tutteli.atrium.api.fluent.en_GB.samples.ThrowableFeatureExtractorSamples$messageFeature2$User <1133787119>)
           ◾ ▶ login: "j"        <44754983>
               ◾ ▶ size: 1        (kotlin.Int <987792975>)
                   ◾ to be greater than: 1        (kotlin.Int <987792975>)
                   ℹ because: login should be longer than one letter
           ◾ ▶ password: "qwerty1"        <1254614872>
               ◾ not to equal: "qwerty"        <952055826>
               ℹ because: password should be secure

The following should probably be hidden:

           ◾ ▶ login: "joe"        <1130906588>
               ◾ ▶ size: 3        (kotlin.Int <439323450>)
                   ◾ to be greater than: 1        (kotlin.Int <987792975>)
                   ℹ because: login should be longer than one letter

...

           ◾ ▶ password: "qwerty1"        <1254614872>
               ◾ not to equal: "qwerty"        <952055826>
               ℹ because: password should be secure

robstoll

The output already looks good 👍 but the implementation needs to be adopted

robstoll · 2023-03-22T07:02:16Z

atrium-core/src/commonMain/kotlin/ch/tutteli/atrium/creating/impl/BaseExpectImpl.kt

@@ -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


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

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)

robstoll · 2023-03-22T07:03:33Z

...ommonMain/kotlin/ch/tutteli/atrium/reporting/text/impl/TextFeatureAssertionGroupFormatter.kt

@@ -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


please revert in the end

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.

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.

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.

robstoll · 2023-03-22T07:08:52Z

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

+ 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
+ }
+ }
+
+ assertion.takeIf { mismatchIf(it.holds()) }?.let {
+ assertionBuilder
+ .feature
+ .withDescriptionAndRepresentation(TranslatableWithArgs(INDEX, index), e)
+ .withAssertion(it)
+ .build()
+ }


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

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.

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

49f50f8

Fixes robstoll#1359

vlsi commented Mar 18, 2023

View reviewed changes

robstoll reviewed Mar 22, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

vlsi commented Mar 18, 2023

vlsi Mar 18, 2023

robstoll Mar 22, 2023 •

edited

vlsi commented Mar 18, 2023 •

edited

robstoll left a comment

robstoll Mar 22, 2023

vlsi Mar 22, 2023

robstoll Mar 22, 2023

vlsi Mar 22, 2023

robstoll Mar 22, 2023 •

edited

robstoll Mar 27, 2023 •

edited

robstoll Mar 22, 2023 •

edited

vlsi Mar 22, 2023

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

Are you sure you want to change the base?

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

Conversation

vlsi commented Mar 18, 2023

vlsi Mar 18, 2023

Choose a reason for hiding this comment

robstoll Mar 22, 2023 • edited

Choose a reason for hiding this comment

vlsi commented Mar 18, 2023 • edited

robstoll left a comment

Choose a reason for hiding this comment

robstoll Mar 22, 2023

Choose a reason for hiding this comment

vlsi Mar 22, 2023

Choose a reason for hiding this comment

robstoll Mar 22, 2023

Choose a reason for hiding this comment

vlsi Mar 22, 2023

Choose a reason for hiding this comment

robstoll Mar 22, 2023 • edited

Choose a reason for hiding this comment

robstoll Mar 27, 2023 • edited

Choose a reason for hiding this comment

robstoll Mar 22, 2023 • edited

Choose a reason for hiding this comment

vlsi Mar 22, 2023

Choose a reason for hiding this comment

robstoll Mar 22, 2023 •

edited

vlsi commented Mar 18, 2023 •

edited

robstoll Mar 22, 2023 •

edited

robstoll Mar 27, 2023 •

edited

robstoll Mar 22, 2023 •

edited