Embrace and promote the use of descriptive messages

[vlsi]

Platform (all, jvm, js): all
Extension (none, kotlin 1.3): none

Code related feature

Currently, it is hard to add descriptive messages to the assertions.
I believe the assertion description should clarify the intention of the test author, so the failure message should include more than "expected: true got: false".

Unfortunately, Atrium examples do not use message at all.
https://github.com/robstoll/atrium#your-first-expectation

val x = 10
expect(x).toEqual(9)

=>

I expected subject: 10        (kotlin.Int <1234789>)
◆ to equal: 9        (kotlin.Int <1234789>)

Why 9 was expected?
When the test fails like that, the developer have to reverse-engineer the meaning of the test.

It would be way better, if the test author supplied the clarification message that describes why something was expected.

For instance

val x = 10
expect(x.coerceIn(0, 9))
    .descibedAs { "$x.coerceIn(0, 9) should be 9" }
    .toEqual(9)

Then the failing test would be way easier to understand.

See more samples in https://kotest.io/docs/assertions/clues.html that I contributed in kotest/kotest#3348

[robstoll]

Thanks for the feature request. Can you please take a look at Attaching a reason and explain the difference to your feature request.

[robstoll]

I can imagine to add functionality which allows to add arbitrary context at any place. however, looking at spring for instance, they just print to the console e.g. when testing with mvcMock the request which were carried out before the failure. do we really want to put context in the expectation error message?

[vlsi]

Even though Spring Boot prints the requests, they do not answer why certain value was expected.

Here's an example:

atrium/atrium-core/src/jsTest/kotlin/ch/tutteli/atrium/reporting/erroradjusters/AdjustStackTest.kt

Lines 45 to 53 in b00b0cb

	fun removeRunner_containsAtriumButNotMochaInCause() {
	val adjuster = assertRemoveRunner(1)._logic.components.build<AtriumErrorAdjuster>()
	expect(adjuster).toBeAnInstanceOf<RemoveRunnerFromAtriumError>()
	val throwable = IllegalArgumentException("hello", UnsupportedOperationException("world"))
	adjuster.adjust(throwable)
	expect(throwable.cause!!.stackBacktrace) {
	it notToContain o entry { it toContain "mocha" }
	it toContain { it toContain "createAtriumError" }
	}

It has two assertions:

1. it notToContain o entry { it toContain "mocha" }
2. it toContain { it toContain "createAtriumError" }

The only explanation clarifying why certain things were expected is given in the method name: removeRunner_containsAtriumButNotMochaInCause.
In this case, it might be enough, however, it might be better to have a way to describe why you expect certain things.

For instance, the following might produce better failure messages .decribedAs("adjuster.adjust(throwable) sould remove .mocha. entries from exception stacktrace") { it notToContain o entry { it toContain "mocha" } }

---

To put it another way: the example expect(10).toEqual(9) does not look like a real-life test.
It just makes no sense to compare 10 and 9.

---

Here's one of my tests.
I do not mean all of them are like that, however, I incline that adding clarifications to all the asserts below is vital.
I incline that adding targeted "feature extractors" would not help much, and it would take time adding such targeted "feature extractors".
Just in case: I compare values in two columns, and I do not really expect there will be targeted "feature extractors" and "comparators" like that.

WDYT?

@Test
fun `overall_info timings`() {
    val mapping = createMapping<Mapping>()
    val res = mapping.applyModifyAndApply {
        nameColumn.dataType = "VARCHAR2(99)"
    }
    assertSoftly {
        for ((instance, result) in res.instanceResults) {
            assertManager.allDatabases().first { it.toString() == instance }.also { db ->
                withClue("Database $db, session_id=${result.sessionId} and explain_plan_uid=hextoraw('${result.explainPlanUid}')") {
                    db.query(
                        """
                        select a.started_when overall_info_started
                             , t.created_when session_created
                             , a.finished_when overall_info_finished
                             , t.last_updated session_last_updated
                             , a.pse_units_total
                             , a.pse_units_started
                             , a.pse_units_completed
                             , a.action_uid
                          from acme_actions a
                             , acme_session_traces t
                         where a.explain_plan_uid = :explain_plan_uid
                           and a.action_type = 'OVERALL_INFO'
                           and t.session_id = :session_id
                        """.trimIndent(),
                        { rs ->
                            if(rs.nextMissing { "OVERALL_INFO / acme_session_trace is not found" }) {
                                return@query
                            }

                            val overallUid = rs.getUid("action_uid")
                            withClue("overall info action_uid=hextoraw('$overallUid')") {
                                withClue("overall_info action's started_when should be the same as session's created_when") {
                                    rs.getObject<OffsetDateTime>("overall_info_started")
                                        .shouldBeWithin(
                                            Duration.ofSeconds(1),
                                            rs.getObject<OffsetDateTime>("session_created")
                                        )
                                }
                                withClue("overall_info action's started_when should be the same as session's created_when") {
                                    rs.getObject<OffsetDateTime>("overall_info_started")
                                        .shouldBeWithin(
                                            Duration.ofSeconds(1),
                                            rs.getObject<OffsetDateTime>("session_created")
                                        )
                                }
                                withClue("overall_info action's finished_when should be the same as session's last_updated") {
                                    rs.getObject<OffsetDateTime>("overall_info_finished")
                                        .shouldBeWithin(
                                            Duration.ofSeconds(1),
                                            rs.getObject<OffsetDateTime>("session_last_updated")
                                        )
                                }
                                withClue("pse_units_completed should match pse_units_total in action") {
                                    rs.getInt("pse_units_completed") shouldBe rs.getInt("pse_units_total")
                                }
                                withClue("pse_units_started should match pse_units_total in action") {
                                    rs.getInt("pse_units_started") shouldBe rs.getInt("pse_units_total")
                                }
                            }
                        },
                        result.explainPlanUid,
                        result.sessionId
                    )
                }
            }
            result.sessionId
        }
    }
    approveExplainPlan(res)
}

[robstoll]

however, it might be better to have a way to describe why you expect certain things.

I guess you missed this comment #1332 (comment)

because is exactly meant to clarify the reason why you expect certain things. but it's not intended to add arbitrary context to reporting this way / neither for grouping.

That being said, I see your need, especially if you need to use a test runner which does not allow to nest test cases. I usually describe things via test-runner. But since, gradle switched reporting in 7.x (or maybe also in 6.x) and now only shows the last leaf in a nested test if it fails on the terminal, this might be even beneficial for test runners which allow nesting as it is quite cumbersome to see in CI which test case failed (in contrast to intellij which shows the hierarchy).

I intend to switch from spek to kotest so that we can run test also for JS and maybe even more targets in the future. As kotest does unfortunately not support nesting when using the JS target I will have the same problems as shown in your example and I will definitely add something like group or similar to Atrium. In the end a solution which falls back on grouping via Atrium would be beneficial so that you still have functionality from intellij such as run a single nested test or run only failing etc. but I don't intend to write a test runner from scratch, let's see if I can easily extend kotest for this behaviour in the future. Sorry... I am digressing.

This issue overlaps with the other issue you opened. I suggest we focus on grouping and attaching context to the error report in this issue as they are closely related and the functionality to collect expectations in arbitrary places in the other issue.

To sum up, for me it's not a question if we should add new functionality to Atrium which supports users like you, it's only the question left how we want to add it.

Taking your example, I would use group for:

• "Database $db, session_id=${result.sessionId} and explain_plan_uid=hextoraw('${result.explainPlanUid}')"
• "overall info action_uid=hextoraw('$overallUid')"

and I would use because in all other cases.

Note that Atrium provides ways to modify how types are represented in reporting. It might be worth improving this functionality (expose it via API, guide the user etc.) together with adding functionality to add arbitrary context. I could imagine that you write your asClue over and over again (in different tests) the same way. It might be worth to define it at a single place instead.

I'll take more time to thing about this new feature. I hope this gives already some input to inspire you to think about it as well.

---

Last but not least

To put it another way: the example expect(10).toEqual(9) does not look like a real-life test.
It just makes no sense to compare 10 and 9.

If you have the time, feel free to improve it. I tried to be as simple as possible but I agree, an example which is a bit more realistic would also make it easier to understand the example 👍

[vlsi]

however, it might be better to have a way to describe why you expect certain things.
I guess you missed this comment #1332 (comment)

I put wrong words.
I should have said "it might be better to have something in the given test to describe why you expect certain things"

Once again, you previously said that

however, looking at spring for instance, they just print to the console e.g. when testing with mvcMock the request which were carried out before the failure. do we really want to put context in the expectation error message?

What I say is that adding extra information is helpful even in the case the framework logs requests and responses, and it is still helpful in the case when the test code uses feature extractors.

---

and I would use because in all other cases.

Well, I find that expect ... because ... toEqual reads odd.

expect("filename?")
    .because("? is not allowed in file names on Windows") {
        notToContain("?")
    }

What do you think of

expect("filename?")
    .withDescription("? is not allowed in file names on Windows") {
        notToContain("?")
    }

expect("filename?")
    .withReason("? is not allowed in file names on Windows") {
        notToContain("?")
    }

---

Inspired by https://github.com/melix/jdoctor

expect(pizza) {
    "Italian cuisine must be respected".so {
        notToContain("pineapple")
    }
}

[robstoll]

Once again, you previously said that

not sure what you mean by that (Edit: I think now I get it, you refer to the next section about spring and not the statement above it) but we both agree that it is valuable to have something in the given test to describe why you expect certain things 👍
did I miss something?

---

What I say is that adding extra information is helpful even in the case the framework logs requests and responses, and it is still helpful in the case when the test code uses feature extractors.

I totally agree, we have already because and we can help by adding group and I think even something like withContext which allows to add arbitrary information at places in the report where the user wants can be more helpful than taking the spring approach and suggest to the user to use println as well (which IMO is a good workaround until Atrium has the feature)

we are on the same page here, isn't it?

please take a look at robstoll/atrium-roadmap#91 (comment)

in the long run I plan to make the distinctions mentioned there.

---

Well, I find that expect ... because ... toEqual reads odd.

I expect the driver, because you have to be an adult to drive, is at least 18 years old.

IMO a perfect sentence, transforms in atrium to:

expect(driver.age).because(...) {
  toBeGreaterOrEqualTo(18)
}

which later on, I would read as:

I expect the driver's age, because you have to be an adult to drive, to be greater or equal to 18.

A bit more technical than the initial thought but
IMO still much closer and more natural than your suggestions.

[robstoll]

btw. thank you very much for your example, it shows that Atrium needs a functionality which allows to compare features, I am going to create a separate issue later on. my goal is that you don't need to use because for things like:

withClue("pse_units_completed should match pse_units_total in action") {
     rs.getInt("pse_units_completed") shouldBe rs.getInt("pse_units_total")
}

because you just repeat what you expect

[vlsi]

because you just repeat what you expect

Even though I repeat, it is way easier to write than figuring out the APIs with "feature extraction".

The mental task is "pse_units_completed should match pse_units_total".
I have two columns: pse_units_completed and pse_units_total.
The comparison logic is expect(rs.getInt("pse_units_completed")).toEqual(rs.getInt("pse_units_total")).
That is it.
Of course, in this case, there's a duplication, however, I do not care. I spend a couple of seconds and move on.

I would not invest time in designing an API that compares features.

I expect the driver, because you have to be an adult to drive, is at least 18 years old.

because in the middle somewhat breaks the sentence though.

[robstoll]

would you still use because if you could write

expect(rs).getInt("pse_units_completed")).toEqual(rs.getInt("pse_units_total"))

and in the report you would see:

I expected...
-> pse_unit_completed: 20
    to equal: 19

the only thing missing is that 19 corresponded to pse_units_total

because in the middle somewhat breaks the sentence though

I know, it's due to a technical reason. if you use because outside an expectation block (which has soft assertions semantics, i.e. without has fail fast semantics), then using because afterwards it's useless for reporting, as it would not show up. butI guess you are aware of that as you ran into the same issue with the asClue. I was actually considering adding the possibility to use it afterwards in case you are inside an expectation block but it requires a breaking change and it is not that important that a breaking change is justified. yet, maybe I'll add it to a version which has breaking chances already

[vlsi]

the only thing missing is that 19 corresponded to pse_units_total

check overall_info action's finished_when should be the same as session's last_updated

Those values come from different entities, so a mere column name is not enough.
Oracle DB has a limit of 30 bytes for the column name, so I can't add arbitrary long messages there.

butI guess you are aware of that as you ran into the same issue with the asClue.

asClue wraps the assertion, so it behaves the same for both soft and regular assertions.

I assume something like this might work:

{ "Here's a common message: $country" }.group {
    expect(pizza).notToContain(pineapple) // the failure will contain "common info"

    expect(pizza) {
        { "pineapple type: ${pineapple.type}" }.group {
            notToContain(pineapple) // the failure should include both top-level group and the nested one
        }
        { "pineapple type: ${pineapple.type}" }.group {
            notToContain(pineapple)
        }
    }
}

I was actually considering adding the possibility to use it afterwards

Adding the message afterward would probably hurt readability in case the comparison spans several lines.

[vlsi]

For reference, here's another case for adding the clarification messages to the failures:

• https://github.com/pgjdbc/pgjdbc/blob/efd36c3f9097f7ee4d7c6f68c83d44e6e16be172/pgjdbc/src/test/java/org/postgresql/test/jdbc2/AutoRollbackTestSuite.java#L300-L315
• https://github.com/pgjdbc/pgjdbc/blob/efd36c3f9097f7ee4d7c6f68c83d44e6e16be172/pgjdbc/src/test/java/org/postgresql/test/jdbc2/BatchFailureTest.java#L213-L233

[robstoll]

asClue wraps the assertion, so it behaves the same for both soft and regular assertions.

Exactly, that's the reason why we did the same with because so that there is no difference between soft and regular assertions and it just works all the time

---

Thanks for those sources. Is your example here #1332 (comment) also available in a public repository? I am interested to see how you use kotest currently

[vlsi]

Is your example here #1332 (comment) also available in a public repository?

Not really :-/

Here's how I migrated it to Atrium. I don't quite like the reporting yet, however, it somehow works.

fun <T> Expect<T>.toBeWithin(
    duration: TemporalAmount,
    other: Temporal
) where T : Temporal, T : Comparable<T> {
    @Suppress("UNCHECKED_CAST")
    val maxExpectedTime = other.plus(duration) as T
    @Suppress("UNCHECKED_CAST")
    val minExpectedTime = other.minus(duration) as T
    _logic.manualFeature(LazyUntranslatable { "Should be within $duration of $other" }) { this }
        .collectAndAppend {
            toBeGreaterThanOrEqualTo(minExpectedTime)
            toBeLessThanOrEqualTo(maxExpectedTime)
        }
}

// Unfortunately, context(Expect<T>) does not quite work here as it breaks the compilation of the test case
// I guess that is a compiler bug
fun <T> Expect<T>.group(description: () -> String, assertionCreator: Expect<T>.() -> Unit): Expect<T> =
    _logic.extractFeature
        .withDescription(LazyUntranslatable(description))
        .withRepresentationForFailure(ErrorMessages.REPRESENTATION_BASED_ON_SUBJECT_NOT_DEFINED)
        .withFeatureExtraction { Some(it) }
        .withOptions {
            withRepresentation { Text.EMPTY }
        }
        .build()
        .collectAndAppend(assertionCreator)

        expectSoftly {
            for ((instance, result) in res.instanceResults) {
                assertManager.allDatabases().first { it.toString() == instance }.also { db ->
                    group({ "Database $db, session_id=${result.sessionId} and explain_plan_uid=hextoraw('${result.explainPlanUid}')" }) {
                        db.query(
                            """
                            select a.started_when overall_info_started
                                 , t.created_when session_created
                                 , a.finished_when overall_info_finished
                                 , t.last_updated + interval '120' minute session_last_updated
                                 , a.pse_units_total
                                 , a.pse_units_started+3 pse_units_started
                                 , a.pse_units_completed
                                 , a.action_uid
                              from acme_actions a
                                 , acme_session_traces t
                             where a.explain_plan_uid = :explain_plan_uid
                               and a.action_type = 'OVERALL_INFO'
                               and t.session_id = :session_id
                            """.trimIndent(),
                            { rs ->
                                if (rs.nextMissing { "OVERALL_INFO / acme_session_trace is not found" }) {
                                    return@query
                                }

                                val overallUid = rs.getUid("action_uid")
                                group({ "overall info action_uid=hextoraw('$overallUid')" }) {
                                    group("overall_info action's started_when should be the same as session's created_when") {
                                        expect(rs.getObject<OffsetDateTime>("overall_info_started"))
                                            .toBeWithin(
                                                Duration.ofSeconds(1),
                                                rs.getObject<OffsetDateTime>("session_created")
                                            )
                                    }
                                    group("overall_info action's finished_when should be the same as session's last_updated") {
                                        expect(rs.getObject<OffsetDateTime>("overall_info_finished"))
                                            .toBeWithin(
                                                Duration.ofSeconds(1),
                                                rs.getObject<OffsetDateTime>("session_last_updated")
                                            )
                                    }
                                    group("pse_units_completed should match pse_units_total") {
                                        expect(rs.getInt("pse_units_completed"))
                                            .toEqual(rs.getInt("pse_units_total"))
                                    }
                                    group("pse_units_started should match pse_units_total") {
                                        expect(rs.getInt("pse_units_started"), "test")
                                            .toEqual(rs.getInt("pse_units_total"))
                                    }
                                }
                            },
                            result.explainPlanUid,
                            result.sessionId
                        )
                    }
                }
                result.sessionId
            }
        }
        approveExplainPlan(res)
    }

Here's a sample failure:

◆ ▶ Database abc, session_id=9166009941513436731 and explain_plan_uid=hextoraw('F5625E984CCF5EF6E0533100DE0A06F4'): 
    ◾ ▶ overall info action_uid=hextoraw('F5625B135FC45EACE0533100DE0ABD2F'): 
        ◾ ▶ overall_info action's finished_when should be the same as session's last_updated: 
            ◾ ▶ I expected: 2023-02-23T20:30:42.618434+03:00
                ◾ ▶ Should be within PT1S of 2023-02-23T22:30:42.612412+03:00: 2023-02-23T20:30:42.618434+03:00
                    ◾ to be greater than or equal to: 2023-02-23T22:30:41.612412+03:00
        ◾ ▶ pse_units_started should match pse_units_total: 
            ◾ ▶ I expected: 24
                ◾ to equal: 21

It would probably be better to have something like (let's ignore "expected should align actual" issue for now)

◆ ▶ Database abc, session_id=9166009941513436731 and explain_plan_uid=hextoraw('F5625E984CCF5EF6E0533100DE0A06F4'):
    ◾ ▶ overall info action_uid=hextoraw('F5625B135FC45EACE0533100DE0ABD2F'):
        ◾ ▶ overall_info action's finished_when should be the same as session's last_updated:
            ◾ ▶ I expected rs.getObject<OffsetDateTime>("overall_info_finished"): 2023-02-23T20:30:42.618434+03:00
                ◾ ▶ Should be within Duration.ofSeconds(1) (PT1S) of rs.getObject<OffsetDateTime>("session_last_updated") (2023-02-23T22:30:42.612412+03:00):
                    ◾ to be greater than or equal to minExpectedTime: 2023-02-23T22:30:41.612412+03:00
        ◾ ▶ pse_units_started should match pse_units_total:
            ◾ ▶ I expected rs.getInt("pse_units_started"): 24
                ◾ to equal rs.getInt("pse_units_total"): 21

[robstoll]

What's the reason that you don't use because but always group?

I think we could get the report to something like the following -- and in this case we skip a few because:

# Database abc, session_id=9166009941513436731 and explain_plan_uid=hextoraw('F5625E984CCF5EF6E0533100DE0A06F4'):
  # overall info action_uid=hextoraw('F5625B135FC45EACE0533100DE0ABD2F'):
    ◆ I expected: ResultSet(....)
      ◾ ▶ column overall_info_finished: 2023-02-23T20:30:42.618434+03:00
          ◾ to equal (error ± PT1S) column session_last_updated:  2023-02-23T22:30:42.612412+03:00
      ◾ ▶ column pse_units_started: 24
           ◾ to equal column pse_units_total: 21

But we would need to introduce a functionality to use features on the right hand side of expectation functions. Once we have it and feature extractors for Expect<ResultSet>.getInt/getObject etc. you could re-write your expectations as follows
(I guess you made some copy&paste mistake as the overall_info action's started_when should be the same as session's created_when is repeated 3 times)

....
if (rs.nextMissing { "OVERALL_INFO / acme_session_trace is not found" }) {
    return@query
}
expect(rs) {
    val overallUid = rs.getUid("action_uid")
    group({ "overall info action_uid=hextoraw('$overallUid')" }) {
        getObject<OffsetDateTime>("overall_info_started")
            .using({ getObject<OffsetDateTime>("session_created") }) { session_created ->
                toEqualWithErrorTolerance(session_created, Duration.ofSeconds(1))
            }

        getObject<OffsetDateTime>("overall_info_finished")
            .using({ getObject<OffsetDateTime>("session_last_updated") }) { session_last_updated ->
                toEqualWithErrorTolerance(session_last_updated, Duration.ofSeconds(1))
            }

        getInt("pse_units_completed")).using({ getInt("pse_units_total") }, ::toEqual)
        getInt("pse_units_started").using({ getInt("pse_units_total") }, ::toEqual)
    }
}

I renamed your toBeWithin to toEqualWithErrorTolerance as we already use this naming for float and double comparison
Most of the time I compare DateTime like types of the db I am also using an error tolerance of 1 second. I can imagine that we even add a shortcut function toEqualWith1SecondErrorTolerance. Then it could be further simplified to

expect(rs) {
    val overallUid = rs.getUid("action_uid")
    group({ "overall info action_uid=hextoraw('$overallUid')" }) {
        getObject<OffsetDateTime>("overall_info_started")
            .using({ getObject<OffsetDateTime>("session_created") }, ::toEqualWith1SecondErrorTolerance)

        getObject<OffsetDateTime>("overall_info_finished")
            .using({ getObject<OffsetDateTime>("session_last_updated") }, ::toEqualWith1SecondErrorTolerance)

        getInt("pse_units_completed")).using({ getInt("pse_units_total") }, ::toEqual)
        getInt("pse_units_started").using({ getInt("pse_units_total") }, ::toEqual)
    }
}

WDYT? especially regarding:

• usage of # as group bullet point?
• the possibility to use features on the right hand side of an expectation via the using approach?

[vlsi]

What's the reason that you don't use because but always group?

The current because has two issues for me:

1. It breaks the mind flow. It sounds weird, and it is weird to write code in a way of expect(..).because(..) { .... }
   The way I write assertions is mostly expect(...).toEqual("23"). Then I add a clarification message based on the bits that will be missing in the automatically generated failure messages.
   I rarely start with a clarification message, and if I write a clarification message right after expect(...), then it would break the flow, and I would have to recover to write the actual code for "expected value comparison".

2. Adding/removing because requires re-adjusting the code

expect(actual)
    .toEqual(expected)

// <=>

expect(actual)
    .because(message) {
        toEqual(expected)
    }

// Adding "group" does not require adjusting the expectation:
group(message) {
    expect(actual) // this is the same with and without the group
        .toEqual(expected)
}

I can't "just add because" as I have to adjust .toEqual(..)

When I use group, I can either skip it, or I can wrap expectations into a group.

As I said earlier, pse_units_started should match pse_units_total probably adds no value (for now it just duplicates column names), so I could just remove that group call as soon as Atrium could automatically add expression texts for expected and actual values.

3. group around the expectation looks like a code comment that is both explaining what is the expectation (like a regular code comment would do), and it gets added to the failure message at the same time

[robstoll]

thanks for the clarification, I see your point about

When I use group, I can either skip it, or I can wrap expectations into a group.

Note, in your example you don't have to use because after expect as you are within an expectation-block already due to expectSoftly. Or in other words, you can also write:

because(message) {
    expect(actual) // this is the same with and without the group
        .toEqual(expected)
}

The difference between group and because will only be visible in reporting.

[vlsi]

That is fair. I probably need to rename expectSoftly to group, then it would be extremely consistent :)

However, having because inside toEqual would look a much better place for because:

expect(actual)
    .toEqual(expected) {
        because(message) // reported as an annotation below "to equal..."
    }

// vs 

group(message) { // reported as group, with expectations inside
    expect(actual)
        .toEqual(expected)
}

// or even
group(message) { // reported as group, with expectations inside
    expect(actual)
        .toEqual(expected) {
            because(extraReason) // reported as an annotation below "to equal..."
        }
}

[vlsi]

I guess you made some copy&paste mistake as the overall_info action's started_when should be the same as session's created_when is repeated 3 times

Thanks. They should be removed.

[vlsi]

usage of # as group bullet point?

It looks good.

the possibility to use features on the right hand side of an expectation via the using approach?

It looks more like a workaround of Kotlin limitations rather than a natural way of writing test to me 🤷

getObject("overall_info_started")

Adding lots of get.. to Expect<ResultSet> might work, however, I was not prepared to add that many extensions before trying what a compiler plugin can do.

For instance, I think regular expect(..).toEqual(...) looks better than the one with using

// Assume compiler plugin will add <<rs.getInt("pse_units_completed")>> and <<rs.getInt("pse_units_total")>> messages
expect(rs.getInt("pse_units_completed"))
    .toEqual(rs.getInt("pse_units_total"))

// getInt looks ok. It requires creating extensions though
getInt("pse_units_started")
     // this looks odd, and it is cumbersome to write
    .using({ getInt("pse_units_total") }, ::toEqual)

It might be interesting to automatically generate Expect<ResultSet> extensions based on ResultSet interface though.
Then it would be easier to maintain. On the other hand, if the extensions are autogenerated, then their error reporting might be suboptimal.

[robstoll]

That is fair. I probably need to rename expectSoftly to group, then it would be extremely consistent :)

More related to #1330, I'll continue this aspect there (glad you mention it)

However, having because inside toEqual would look a much better place for because:

I guess I am a marked man when it comes to giving reasons why something failed. I have seen so much repetition in the past and in so many cases those repetitions where not updated when the corresponding expectations were changed (same problem as with code comments). That was the reason why I did not include something like a message: String parameter in expectation functions. Yet, taking a step back (overcoming the trauma 😆) I think this would be the most intuitive and clean solution:
expect(x).toEqual(3, because = "bla bla bla") which would look like the following in reporting:

I expected subject: 4
◆ to equal: 3 
ℹ because: bla bla bla

and then you would use because { ... } only if you have want to state several expectations something like:

expect(x).because(""bla bla bla"){
   toBeGreaterThan(5)
   toBeLessThan(10)
}

which in reporting looks like

I expected subject: 4
◆ to be greater than: 5
◆ to be less than: 10
ℹ because: bla bla bla

and we could think about adding a because which is only available within expectation groups:

expect(x) {
   toBeGreaterThan(5)
   toBeLessThan(10)
   because(""bla bla bla")
}

WDYT?

---

Adding lots of get.. to Expect might work,

As ResultSet is very stable, I wouldn't mind to add feature extractors manually (and not generate them).

The advantage of feature extractors compared to compiler plugin would be:

• cleaner reporting than including code in the error report
• possibility to add failure hints (e.g. show all column names, maybe the developer made a typo?)
• catching exceptions (though we could and should do this in the compiler plugin as well) and showing it. For instance, if the column has a different type.

Nevertheless, I think a compiler plugin would be a nice addition, especially for folks which primarily want a clean way to write the expectation functions and don't mind if the report contains code, does not provide failure hints etc.

For instance, I think regular expect(..).toEqual(...) looks better than the one with using

I agree, it looks better. I was considering in the past to generate overloads for expectation functions via KSP where we expect Named<T> instead of T which then allows the user to write something like:

expect(x).toEqual(named("my number", 1))

We could take the same approach and add overloads for feature extractors. Then it would look as follows.

expect(rs) {
   getInt("pse_units_started").toEqual { getInt("pse_units_total") } )
}

This would actually be way cleaner than the using approach. I just don't like that we clutter the API with overloads (also the reason why I did not pursue with the Named idea) and in the end the user isn't sure which one he should take, struggling with the API. A pity that Kotlin does not support union types 😢

[vlsi]

Initially I created toBeWithin / toEqualWithErrorTolerance in the same way

expect(x).because(""bla bla bla"){
   toBeGreaterThan(5)
   toBeLessThan(10)
}

However, it produced bad reports: it does not print print expectations that hold, and it was hard to understand the actual expectation was between.

So I moved to manualFeature that explicitly mentions "should be between" message.

---

which in reporting looks like

I expected subject: 4
◆ to be greater than: 5
◆ to be less than: 10
ℹ because: bla bla bla

Currently, less than 10 won't be displayed :-/

---

The advantage of feature extractors compared to compiler plugin would be...
Nevertheless, I think a compiler plugin would be a nice addition

Those are my thoughts exactly.

[robstoll]

However, it produced bad reports: it does not print print expectations that hold, and it was hard to understand the actual expectation was between.
Currently, less than 10 won't be displayed :-/

True my mistake. Your experience using Atrium is very valuable to understand what features we should enable in the API. Atrium shows only failing expectations by default for most expectation functions. But there is already a SummaryGroup functionality on the logic level which allows to show also expectations which hold. I am going to create an issue later on => #1342

[vlsi]

For reference, I've added the compiler plugin id("io.github.vlsi.kotlin-argument-expression") version "1.0.2" to my test code, and I have the following results:

group({ "overall info action_uid=hextoraw('$overallUid')" }) {
    expect(rs.getObject<OffsetDateTime>("overall_info_started"))
        .toEqualWithErrorTolerance(
            rs.getObject<OffsetDateTime>("session_created")
        )
    expect(rs.getObject<OffsetDateTime>("overall_info_finished"))
        .toEqualWithErrorTolerance(
            rs.getObject<OffsetDateTime>("session_last_updated"),
            Duration.ofSeconds(1)
        )
    expect(rs.getInt("pse_units_completed"))
        .toEqual(rs.getInt("pse_units_total"))
    expect(rs.getInt("pse_units_started"))
        .toEqual(rs.getInt("pse_units_total"))
}

◆ ▶ Database odb, session_id=9166043183413493365 and explain_plan_uid=hextoraw('F5AFC5395EC92340E0533100DE0A2527'): 
    ◾ ▶ overall info action_uid=hextoraw('F5AFC0F488742317E0533100DE0A1155'): 
        ◾ ▶ I expected rs.getObject<OffsetDateTime>("overall_info_started"): 2023-02-27T16:51:05.696328+03:00
            ◾ ▶ to equal <<getObject<OffsetDateTime>("session_created")>> with PT1S tolerance <<>>: 2023-02-27T16:51:05.696328+03:00
                ◾ to be greater than or equal to: 2023-02-27T17:01:04.696328+03:00
        ◾ ▶ I expected rs.getObject<OffsetDateTime>("overall_info_finished"): 2023-02-27T16:51:16.091049+03:00
            ◾ ▶ to equal <<getObject<OffsetDateTime>("session_last_updated")>> with PT1S tolerance <<ofSeconds(1)>>: 2023-02-27T16:51:16.091049+03:00
                ◾ to be greater than or equal to: 2023-02-27T18:51:15.085340+03:00
        ◾ ▶ I expected rs.getInt("pse_units_completed"): 21
            ◾ to equal: 20
            ℹ because: rs.getInt("pse_units_total")
        ◾ ▶ I expected rs.getInt("pse_units_started"): 24
            ◾ to equal: 20
            ℹ because: rs.getInt("pse_units_total")

Well, because looks a bit odd, however, in general it works.
It would be nice if there was a possibility to squeeze "subject description" in-beween to equal: 20 like to equal <<..description...>>: actualValue

Here are some extensions:

fun <T> expect(
    subject: T,
    @CallerArgumentExpression("subject") description: String = ""
): RootExpect<T> =
    RootExpectBuilder.forSubject(subject)
        .withVerb(LazyUntranslatable { "I expected $description" })
        .withOptions {
            withComponent(ObjectFormatter::class) { c -> SimplifiedObjectFormatter(c.build()) }
        }
        .build()

fun <T> expect(
    subject: T,
    @CallerArgumentExpression("subject") description: String = "",
    assertionCreator: Expect<T>.() -> Unit
): Expect<T> =
    expect(subject, description).and(assertionCreator)

fun <T, R> Expect<T>.expect(
    newSubject: R,
    @CallerArgumentExpression("newSubject") description: String = ""
): FeatureExpect<T, R> =
    _logic.manualFeature(LazyUntranslatable { "I expected $description" }) { newSubject }
        .transform()

fun <T> Expect<T>.toEqual(
    expected: T,
    @CallerArgumentExpression("expected") description: String = ""
): Expect<T> =
    if (description.isNotEmpty()) {
        because(description) {
            toEqual(expected)
        }
    } else {
        toEqual(expected)
    }

fun <T> Expect<T>.toEqualWithErrorTolerance(
    other: Temporal,
    tolerance: TemporalAmount = Duration.ofSeconds(1),
    @CallerArgumentExpression("other") otherDescription: String = "",
    @CallerArgumentExpression("tolerance") toleranceDescription: String = "",
) where T : Temporal, T : Comparable<T> {
    @Suppress("UNCHECKED_CAST")
    val maxExpectedTime = other.plus(tolerance) as T

    @Suppress("UNCHECKED_CAST")
    val minExpectedTime = other.minus(tolerance) as T
    _logic.manualFeature(LazyUntranslatable { "to equal <<$otherDescription>> with $tolerance tolerance <<$toleranceDescription>>" }) { this }
        .collectAndAppend {
            toBeGreaterThanOrEqualTo(minExpectedTime)
            toBeLessThanOrEqualTo(maxExpectedTime)
        }
}

[robstoll]

looks promising 👍

Well, because looks a bit odd, however, in general it works.
It would be nice if there was a possibility to squeeze "subject description" in-beween to equal: 20 like to equal <<..description...>>: actualValue

I agree

[vlsi]

Here's a relevant review by Truth team: google/truth#849

[robstoll]

@vlsi Thanks for the link, interesting read.
I don't see it mentioned in this issue although I am pretty sure I communicated my idea somewhere. I was thinking to introduce a because parameter in every expectation function (a lot of work initially but we don't have hundreds of functions yet) so that one can write:

expect(filename)
  .notToContain("?", because = "? is not allowed in file names on Windows")

and I still think it would be helpful if Atrium had a compiler plugin which turned:

expect(person) {
   its { name }.toEqual("robert")
}

In reporting into:

I expected subject: Person(...)
-> name: "Peter"
   - to equal: "Robert"

But that's more the task of robstoll/atrium-roadmap#74 (I guess I should turn those issues into discussions in Atrium, now that I have enabled this feature it would be a better fit to have everything in one place).

---

btw. Atrium 1.1.0 shipped with expectGrouped including nesting expect and group (see https://github.com/robstoll/atrium#ex-data-driven-nesting as an example)