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.

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

Add soft assertions for verifying unrelated subjects #1330

Closed
vlsi opened this issue Feb 16, 2023 · 19 comments
Closed

Add soft assertions for verifying unrelated subjects #1330

vlsi opened this issue Feb 16, 2023 · 19 comments
Assignees
Milestone

Comments

@vlsi
Copy link
Collaborator

vlsi commented Feb 16, 2023

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:

  • group for creating a group for output purposes. In other words, it should be included in the failure messages, however, it does not correspond to a subject
  • I would like to have multiple expect(..). For instance, I would like to have some of the verifications in @AfterEach, and some of the verifications inside the test method itself.
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

  • ☑️ remove deprecation of expect within an expectation group

in 1.1.0

  • ☑️ add fun expectGrouped(description: String = null) as entry point (including an optional group description)
  • ☑️ add group => both group and expectGroup will have # as bullet point.

I plan in 1.2.0 (or 1.3.0):

@robstoll
Copy link
Owner

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
Copy link
Collaborator Author

vlsi commented Feb 16, 2023

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
Copy link
Owner

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
Copy link
Collaborator Author

vlsi commented Feb 16, 2023

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
Copy link
Collaborator Author

vlsi commented Feb 16, 2023

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
Copy link
Owner

robstoll commented Feb 16, 2023

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 (#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
Copy link
Owner

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
Copy link
Collaborator Author

vlsi commented Feb 17, 2023

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
Copy link
Collaborator Author

vlsi commented Feb 17, 2023

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

  • group descriptions
  • soft failure mode
  • sane composability

I tried several libraries, and got mixed results:

AssertJ:

  • no group descriptions, it is hard to add it
  • soft failure mode somewhat works, however, it has issues
  • it is hard to extend (it uses too much generic hackery caused by Java)

Kotest:

  • has group descriptions
  • soft failure works, error collector is global, so it is trivial to share
  • it is hard to extend. Unfortunately, matchers are written in ad-hoc manner

Now I'm evaluating Atrium

@robstoll
Copy link
Owner

robstoll commented Feb 17, 2023

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
Copy link
Owner

robstoll commented Feb 17, 2023

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:

  • expecting vs. expectGrouped vs. collecting -- for expect without subject intended for use cases as above
  • expectCollector() for the factory method:
    val softly = expectCollector()
    softly.expect(...).toEqual()
     myMethodWithReceiver(softly)
     .... // within method
     softly.expect(...)
     // and maybe even
     softly.expectGrouped { ... }
     // and in the end you would need to call
     softly.expectAll()
    
    Tooling which helps in automatically calling expectAll would need to be provided as well. the factory approach could then also be used for your afterTest example (out of curiosity, what runner are you using which does not support teardown functionality via afterTest in case a test fails?)

@robstoll
Copy link
Owner

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
Copy link
Collaborator Author

vlsi commented Feb 18, 2023

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.

@robstoll robstoll self-assigned this Feb 20, 2023
@vlsi
Copy link
Collaborator Author

vlsi commented Feb 23, 2023

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
Copy link
Owner

@vlsi as mentioned by you in #1332 (comment)

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
Copy link
Collaborator Author

vlsi commented Feb 24, 2023

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
Copy link
Owner

robstoll commented Oct 7, 2023

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
Copy link
Owner

@vlsi I forgot to create the issue for expectCollector back in October 🙈 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 (#1344 (reply in thread))
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
Copy link
Collaborator Author

vlsi commented Jan 31, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants