robstoll / atrium

A multiplatform expectation library for Kotlin
https://docs.atriumlib.org
European Union Public License 1.2
570 stars 211 forks source link

Add soft assertions for verifying unrelated subjects #1330

Closed vlsi closed 1 year ago

vlsi commented 1 year ago

Platform (all, jvm, js): all

I would like to have multiple soft assertions for unrelated subjects.

For instance, here's something I have in mind.

Note the following:

assertSoftly {
    // verify general system sanity
    // These "generic" verifications are included in @AfterEach / @AfteMethod
    for(db in databases) {
        group("Database $db") {
            expect(db.isAlive).toEqual(true)
        }
    }
    // This might be a test-specific verification
    val tableName = "TEST_TABLE"
    group("Verifying table $tableName") {
        // some.expectations.with.tableName
        // other expectations
        for(db in databases) {
            group("Database $db") {
                val con = db.connection
                con.createStatement().use { st ->
                    val rs: java.sql.ResultSet = st.executeQuery("select count(*) from ${st.enquoteIdentifier(tableName, false)}")
                    // it would be nice to reuse "resultset must contain row" matcher here, but it's not possible

                    // How do I add description like "rs.next() for query that counts rows in table"
                    expect(rs.next())
                        .toEqual(true)

                    // How do I add description like "number of rows in table"?
                    expect(rs.getInt(1))
                        .toEqual(0)
                }
            }
        }
    }
}

Frankly speaking, I do not see the way to add "groups" for description purposes.

At the same time, feature API is unexpected, and it deviates from expect(...). In other words, a top-level expect is expect(subject).toEqual(target), while nested feature is feature("description") { subject }.toEqual(target).


Here's the similar requests for other assertion libraries:


Edit by @robstoll

To sum up, I plan in 1.0.0

in 1.1.0

I plan in 1.2.0 (or 1.3.0):

robstoll commented 1 year ago

thanks for the feature request. I think Atrium core has everything needed for your request. but the API and some tooling (like calling the method which evaluates the expectations) is missing. one point I would like to know is how the reporting would look like.

vlsi commented 1 year ago

Oh, the reporting is a different beast. I guess we could better discuss it in a different issue.

Frankly speaking, I have two cases: 1) Small comparisons like "small string compared with another one" 2) Bigger comparisons like "100+ yaml compared with another 100+ line yaml". It would probably work if Atrium could render something like

 ....  execution plan for .. differs, see [2] below

[2]: execution plan for ...
---expected
+++actual
diff-like output
robstoll commented 1 year ago

Sorry, was writing from my mobile and did not formulate a very precise question. In the meantime I had a chance to look at the other two issues and saw the reporting example you made in the assertJ issue. That's good enough for now. I'll get back to you. In the meantime, could you please elaborate more on:

At the same time, feature API is unexpected, and it deviates from expect(...). In other words, a top-level expect is expect(subject).toEqual(target), while nested feature is feature("description") { subject }.toEqual(target).

I don't get what you mean by that. How would you have expected feature to behave?

// it would be nice to reuse "resultset must contain row" matcher here, but it's not possible

I guess that's not Atrium related but sounds like another feature request :) Would you mind to create another issue for that. Thanks

vlsi commented 1 year ago

I don't get what you mean by that. How would you have expected feature to behave?

I mean that when I want to assert something, my mind flows like "I want to assert AAA to be BBB".

So I expect to write code like assertThat(AAA)., then either type something like isEqualTo or wait for autocomplete, then type BBB.

That works good for expect(AAA).toEqual(BBB).

However, feature("...") { AAA } breaks this pattern. I just don't think of "AAA" to be a "feature of something".

Something like the following would be way better

// kotest style, however, I would prefer { ... }.asGroup {...} rather than .asClue
{ "Lazy description with $extra $comments" }.asClue {
    expect(AAA).toEqual(BBB)
}

// or

expect(AAA)
    .withDescription { "Lazy description with $extra $comments" }
    .toEqual(BBB)

// or in AssertJ style
expect(AAA)
    .describedAs { "Lazy description with $extra $comments" }
    .toEqual(BBB)

I believe, clarification messages are extremely important to keep test maintainable, and I am puzzled why Atrium makes it so hard to add grouping/description messages.

vlsi commented 1 year ago

I guess that's not Atrium related but sounds like another feature request :) Would you mind to create another issue for that. Thanks

For Atrium, it would be something like being able to use expect(newSubject).toEqual(...) within a soft assertion block (==this issue)

robstoll commented 1 year ago

thanks for the additional example. I can see that I need to explain features (feature extractors) better in the docs. Its an abstraction over properties, method calls and more. AssertJ has a method extracting for more or less the same purpose.

So whenever you think along the following lines:

val person = businessLogic.createAwesomePerson()
expect(person.firstName).toEqual("Robert")
expect(person.lastName).toEqual("Stoll")

then Atrium encourages you to use feature extractors instead to automatically include the identifiers of your features (in this case of properties) in reporting and, IMO equally important, to give more context by also showing the full subject (person in this example, maybe it has more properties which are also relevant to better understand why a test failed).

Depending whether you want to re-state the subject-type or not you will either use (re-stating the type):

expect(person) {
    feature(Person::firstName).toEqual("Robert")
    feature(Person::lastName).toEqual("Stoll")
}

or the following (not re-stating the type, but syntax which you need to get used to)

expect(person) {
  feature { f(it::firstName) }.toEqual("Robert")
  feature { f(it::lastName) }.toEqual("Stoll")
}

or in case you don't care about firstName/lastName showing up in reporting even the following (easy syntax but the error report is not that good, see also: https://github.com/robstoll/atrium#feature-extractors)

expect(person) {
  its { firstName }.toEqual("Robert")
  its { lastName }.toEqual("Stoll")
}

And the report then looks like (for the first two alternatives)

I expected subject: Person(firstName=Vladimir, lastName=Sitnikov)        (SmokeTest.Person <344816031>)
◆ ▶ firstName: "Vladimir"        <312492472>
    ◾ to equal: "Robert"        <175493193>
◆ ▶ lastName: "Sitnikov"        <253446934>
    ◾ to equal: "Stoll"        <146499426>

I am puzzled why Atrium makes it so hard to add grouping/description messages.

Personally, I use grouping and description of the test-runner library for that purpose but I'll answer in more depth in the other issue you created (https://github.com/robstoll/atrium/issues/1332). Unless we speak about features of the subject, then I use feature extractors of course

Following an approach how you can fulfil most of your wishes (note that expect will be deprecated though as we saw in the past that people nest expect instead of using feature or its

@OptIn(ExperimentalWithOptions::class)
fun expectSoftly(expectations: Expect<Unit>.() -> Unit): Expect<Unit> =
    expect(Unit).withOptions{
        withVerb("I expected for")
        withRepresentation { Text.EMPTY }
    }.and(expectations)

fun <T> Expect<T>.group(groupDescription: String, assertionCreator: Expect<T>.() -> Unit) =
    feature("group: $groupDescription", { this }, assertionCreator)

And then you can write:

expectSoftly {
    val tableName = "TEST_TABLE"
    group("Verifying table $tableName") {
        expect(1).toEqual(2)
        for (db in databases) {
            group("Database $db") {
                expect(2).toEqual(3)
            }
        }
    }
}

The error report then looks as follows:

I expected for: 
◆ ▶ group: Verifying table TEST_TABLE: kotlin.Unit        (kotlin.Unit <655829039>)
    ◾ ▶ I expected subject: 1        (kotlin.Int <629297364>)
        ◾ to equal: 2        (kotlin.Int <348336347>)
    ◾ ▶ group: Database postgres: kotlin.Unit        (kotlin.Unit <655829039>)
        ◾ ▶ I expected subject: 2        (kotlin.Int <348336347>)
            ◾ to equal: 3        (kotlin.Int <319262590>)
    ◾ ▶ group: Database mysql: kotlin.Unit        (kotlin.Unit <655829039>)
        ◾ ▶ I expected subject: 2        (kotlin.Int <348336347>)
            ◾ to equal: 3        (kotlin.Int <319262590>)

Of course this can be improved with a real group implementation (a bit less kotlin.Unit clutter for instance) Note as well, that this does not cover the aspect that you would like to collect expectations in several places (e.g. @AfterEach). IMO questionable if this really need to be soft-assertions. Is the expectation which fails in @AfterEach really important to show together with the expectations which failed in the test itself? Or is it ok, that they are reported separately?

robstoll commented 1 year ago

btw. Atrium has currently no extra support for ResultSet, if you created expectation functions for it, then please share them with us 😊 and in case you need help creating one then let me know

vlsi commented 1 year ago

Following an approach how you can fulfil most of your wishes (note that expect will be deprecated though as we saw in the past that people nest expect instead of using feature or its

Well, you suggest using deprecated APIs, and I'm not fond of having deprecated calls all over the place :-/ That is one of the main reasons for raising the issue.

I see that I can wrap feature into my own function, however, I don't see how can I escape expect deprecation. Well, I can probably borrow its code and create my own non-deprecated assertThat, however, it would look a bit confusing for others maintaining the test 🤷

vlsi commented 1 year ago

IMO questionable if this really need to be soft-assertions. Is the expectation which fails in @AfterEach really important to show together with the expectations which failed in the test itself?

That is interesting question. I can probably configure @AfterTest to execute even if the test fails (typically, it does not execute). So getting all the exceptions in a single go is not that important for me as having something that supports

I tried several libraries, and got mixed results:

AssertJ:

Kotest:

Now I'm evaluating Atrium

robstoll commented 1 year ago

Well, you suggest using deprecated APIs, and I'm not fond of having deprecated calls all over the place :-/ That is one of the main reasons for raising the issue.

Now that I see that there is a real use case, I am going to remove the deprecation with the next release. shouldn't be a blocker. I think you can work around the deprecation in the meantime as follows (writing from mobile, please verify yourself):

in file A:

fun <T> Expect<Unit>.expect(subject: T) = 
//copy from deprecated version 

in test:

import ch.tutteli.atrium.verbs.* // for lower precedence
import com.example.expect

I guess this way your expect will have precedence over the deprecated version. make sure you import it without star.

robstoll commented 1 year ago

I can actually think of situations where I did not use Atrium and soft assertions had to be added in a softAssertion.assertThat style because methods were called which already had receivers. As I still intend to support older kotlin versions (currently we still support kotlin 1.2), so context receivers is not an option. So I'll plan to add something as well. WDYT regarding the following naming proposition:

robstoll commented 1 year ago

btw. did you see https://github.com/robstoll/atrium#data-driven-testing -- it's not like Atrium does not support soft assertions already 🙂 -- but I see that we can improve here further

vlsi commented 1 year ago

expectCollector() for the factory method:

This sounds good to me.

did you see https://github.com/robstoll/atrium#data-driven-testing

Not really. My system under test is a component running inside a database. I supply it with configurations (e.g. table names, column names), and it creates the tables accordingly. 99% of the time, I test the "apply configuration changes" procedure within the database, which takes 1-2 minutes to execute.

That is why I want to run the procedure in the DB once, and then assert multiple things: the expected small changes for the given test, the overall system health, logs, and metrics.

The tests run with TestNG, and the current assertions are like "build a big string with all the assertion material, and then assert with org.testng.Assert.assertEquals". It is suboptimal from the test writing perspective, however, it yields a more-or-less reasonable "click-to-see difference" experience.

vlsi commented 1 year ago

Atrium has currently no extra support for ResultSet, if you created expectation functions for it, then please share them with us 😊

I've prepared some wrappers to reduce verbosity, and I migrated the above withClue assertions to atrium.

Well, I have two issues: 1) ResultSet column accessor has two axes: column name (or position), and accessor method (getInt, getString, or even Kotlin extension method for retrieving a custom type). I believe that makes impractical trying to come up with a fancy extractor like expect(rs).column("name"). Those extractors will be non-trivial to use, and it won't make the output dramatically better 2) It would be nice to have an API to peek on the current state of Expect<T> to skip subsequent soft assertions when they make no sense. For instance, the first thing I check is rs.next(), and I need skip processing when there's no data in the resultset. What is the idiomatic way to do that? expect(rs.next()).toEqual(true)? How do I know if the expectation holds? For now, I use if (!rs.next()) { _logic.append(.."no rows found for.."); continue}, however, it looks weird

I'm going to try a compiler plugin so expect(rs.getString("name")) would yield report like "I expect rs.getString("name"): actual value..."

robstoll commented 1 year ago

@vlsi as mentioned by you in https://github.com/robstoll/atrium/issues/1332#issuecomment-1443454066

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

I intend to add this entry-point to atrium as well. Currently I was considering one of the following:

expecting vs. expectGrouped vs. collecting

expectSoftly{ ... } looks to close to expec { ... } but would have a completely different meaning, that's why I did not include this in the list.

I kind of like the idea that a user of Atrium always has to write expect as entry point. It is then up to the user to decide how to continue (with the help of code completion), either with (subject) or { ... } or Grouped/ing (resulting in expect(subject) or expect { ... } or expectGrouped { ... }/expecting{ ... })

I am currently in favour of expectGrouped { ... } and reading your comment above, it seems the right choice. WDYT?

vlsi commented 1 year ago

I am currently in favour of expectGrouped { ... } and reading your comment above, it seems the right choice. WDYT?

expectGrouped enables having a top-level (optional) group comment which is nice

I agree it is nice to type expect, and then decide how to proceed.

robstoll commented 1 year ago

I will move expectCollector to a separate issue because the main topic of this issue, make it simpler to create expectations for unrelated subjects is solved now with expectGrouped

robstoll commented 9 months ago

@vlsi I forgot to create the issue for expectCollector back in October :see_no_evil: Now that I think about it again I am not yet fully convinced we really need it. So I wonder, do you still have a use case for it where you actually want to collect failures in the test and add further test failures caused by an afterTest (or similar) callback, now that Atrium provides expectGrouped? You wrote in a previous comment that it's not that important but things might have changed since then

I first figured expectCollector could be helpful if you want to pass Expect around to collect assertions and in the end evaluate if it worked but with expectCollector there is always the risk that someone forgets to call expectAll in the end -- i.e. the test always passes. Am thus very hesitant to introduce it. I think, it would be better if we enable passing expect around in another way, something along the lines of:

expect {
    setupTestDsl()
}.notToThrow {
    extractSubject { dsl ->
        dsl.verifyGenericTestLogic(this)
    }
}

The only missing piece here is extractSubject and I remember from the discussion about ResultSet, that we decided to introduce something like it (https://github.com/robstoll/atrium/discussions/1344#discussioncomment-5343932) So there would be use for it also in other contexts. And I think it would even make sense to provide a shortcut notToThrowAndExtractSubject

Of course, this would not help if someone really want to state expectations in an @Before/AfterTest or the like. But maybe we can neglect this and ask users which really need this to create a CollectingExpect manually? Did you have such a use case lately?

vlsi commented 9 months ago

Creating CollectingExpect manually sounds fine to me.

expectCollector is not pressing for me now. At worst, I can have several expectGrouped calls one after another or create CollectingExpect.