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

A syntax that can allow grouping of assertions without compounding #805

Open
StragaSevera opened this issue Feb 8, 2021 · 11 comments
Open

Comments

@StragaSevera
Copy link

Related to: #790

I think that the syntax where you pass a lambda to the expect function is useful, but there may be cases when you just want to group assertions without the need of compounding them - when it's perfectly OK to fail on the first failing assertion.

I propose something like this:

expect(person).all { 
  its { firstName }.toBe("Ilya")
  its { lastName }.toBe("Fedorinov")
}

Of course, the function name (all) is just the first name that I thought of.

@jGleitz
Copy link
Collaborator

jGleitz commented Feb 8, 2021

  1. I think all is not a good name because we already have Expect<Iterable<T>>.all.
  2. What is your use case that makes you want to avoid fail-late behaviour?

@StragaSevera
Copy link
Author

What is your use case that makes you want to avoid fail-late behaviour?

As we discussed in the #790, it will be desirable with the shortened feature testing syntax. When you will not have the ability to easily see which assertion from the group has failed, making the stacktrace point to a correct assertion is very useful.

@robstoll
Copy link
Owner

robstoll commented Feb 8, 2021

How about the following:

expect(person) { 
  failFast = true
  its { firstName }.toBe("Ilya")
  its { lastName }.toBe("Fedorinov")
}

where it does not matter where you place failFast within the block, i.e. it always affects the whole block

@jGleitz
Copy link
Collaborator

jGleitz commented Feb 8, 2021

making the stacktrace point to a correct assertion is very useful.

I agree with this. Not only for the shortened feature syntax, but in general. However, I think it would be great to brainstorm whether we can come up with a solution that does not require sacrificing fail-late behaviour. Because I think that’s one of atrium’s strengths: If multiple expectations fail, I don’t have to run my tests multiple times, I can see it straight away in the first run.

So my question is: Can we find a solution that will be available for users that want to keep fail-late behaviour, as well?

One possibility would be to have the main exception’s stack trace point to the first failing expectation, and add suppressed exceptions for every other failing expectation. This way, we would have a pointer for every failing expectation.

But maybe we can even come up with a way for atrium to report the location of failed expectations. That way should support links in IDEs, though.

@robstoll
Copy link
Owner

robstoll commented Feb 8, 2021

As info, I intend to improve its so that it will be able to show its content -- but at a later point. IMO this feature is not that crucial. I'll probably start off with using https://github.com/christophsturm/filepeek and only do it for JVM -- I have not done that so far because filepeek uses file IO which can be shaky for tests and is rather slow. That being said, I will look into a compiler plugin once Kotlin 1.5 is out where I expect the IR to be stable (or wait until it is stable). This would be the better alternative but I want to have a nice cost/maintenance balance in this regard.

But maybe we can even come up with a way for atrium to report the location of failed expectations. That way should support links in IDEs, though.

That's something I had in mind but dismissed as we have this info in the stacktrace and I figured it will clutter the report. But I think it makes sense to revisit this idea for its. We could add the location as debug info to the error message. Something like:

expected that the subject: Person(firstName="Robert", lastName="Stoll")
* -> feature 1: "Robert"
      * to be : "Ilya"
      🔎 com.example.MyTest.methodXy(MyTest.kt:26)
* -> feature 2: "Stoll"
      * to be : "Fedorinov"    
      🔎 com.example.MyTest.methodXy(MyTest.kt:27)

As far as I can see, IntelliJ would turn it into a clickable link. The only problem here, we don't have the location and we would again need to resort to filepeek/compiler plugin or a different solution.

Edit: We could also get the line number by creating a stacktrace. Slow as well but already better than filepeek

Anyway, what I would like to know from you two. Do you think such a feature would be valuable also in other cases where we actually have the feature name?

@jGleitz
Copy link
Collaborator

jGleitz commented Feb 8, 2021

We could also get the line number by creating a stacktrace.

How would you find out which stack trace entry to take?

Do you think such a feature would be valuable also in other cases where we actually have the feature name?

Yes, I can imagine this feature being useful in other scenarios as well. I am mainly thinking of larger tests where it’s not directly obvious where the expectation is located at.

However, I could imagine that it would be better to have the information below the report (like we already have the stacktrace below the report) because it clutters the report less.

Having said that, I also think that it’s worth considering my idea to use the suppressed expectations to report the other locations. If we filter the stack traces well, it might be a cheap but useful solution.

@robstoll
Copy link
Owner

robstoll commented Feb 8, 2021

How would you find out which stack trace entry to take?

E.g. by creating an exception when creating the feature extraction. The bad thing about it, we would need to create it also if the feature holds and creating exceptions is slow. Still better than filepeek which does this as well but file IO in addition. I would prefer to wait until the compile plugins is stable where such a feature should be fast.

Having said that, I also think that it’s worth considering my idea to use the suppressed expectations to report the other locations. If we filter the stack traces well, it might be a cheap but useful solution.

if we put it below, then I would also go with the stacktrace since this is already familiar to the developer. However, we should use
chained causes instead of suppressed exceptions since most tooling simply ignore suppressed exceptions (at least when I checked last time). A different solution would be to manipulate the stacktrace (we already filter) so that the different errors follow one after another. Something like:

ch.tutteli.atrium.reporting.AtriumError
	at com.example.MyTest.methodXy(MyTest.kt:26)
	at com.example.MyTest.methodXy(MyTest.kt:27)

Where we sneaked in at com.example.MyTest.methodXy(MyTest.kt:27)

@jGleitz
Copy link
Collaborator

jGleitz commented Feb 8, 2021

E.g. by creating an exception when creating the feature extraction.

Yeah, I got that, but which of the stack trace’s entries is the one to show to the user, if you want to select only one? Users might compose expectations out of other expectations, so a simple algorithm like ‘the first element that doesn’t belong to atrium’ will often not produce useful results. If we can’t get this right, we should prefer showing filtered stack traces, I think.

most tooling simply ignore suppressed exceptions

That is not my experience at all. Which kind of tooling are you referring to? IntelliJ prints suppressed exceptions (simply because the JVM prints them). Suppressed exceptions are, for example, quite common in Project Reactor.

we should use chained causes

I don’t like this idea because it is semantically wrong. A cause means ‘this exception only happened because of this other exception’, which is not true if multiple expectations fail. The semantic of suppressed exceptions, on the other hand, is ‘we would have shown you that other exception if we weren’t already processing this one’, which is exactly right for multiple failing expectations.

A different solution would be to manipulate the stacktrace

Interesting idea, however, I fear that it might irritate users a lot. The usual semantics of stack traces is to represent the stack. Listing multiple causing code lines is unintuitive. Also, we’d have the problem again that we need to identify ‘the one right stack trace element’ which will, as I wrote above, be difficult.

@jGleitz
Copy link
Collaborator

jGleitz commented Feb 8, 2021

@StragaSevera would one of these solutions address your use case? Or would you still want to have a way to make lambda blocks fail-fast? If yes, why?

@robstoll
Copy link
Owner

robstoll commented Feb 8, 2021

That is not my experience at all.

I remember that logging libraries where not showing it. But I was surprised to see lately, that they are now including it nowadays (which is good). So maybe my knowledge also outdated here

IntelliJ prints suppressed exceptions

I did a quick check, it seems to be printed when using spek 🎉
However, it is not shown with Junit5 (probably the same for junit4 where my knowledge comes from) even with the following config.

   exceptionFormat TestExceptionFormat.FULL
        showExceptions true
        showCauses true
        showStackTraces true

Maybe a mistake on my part 🤷‍♂️ on the other hand, if the default does not show it, then it is not a solution IMO.

@jGleitz
Copy link
Collaborator

jGleitz commented Feb 8, 2021

it is not shown with Junit5

That’s a bummer.

if the default does not show it, then it is not a solution

I’m not sure about this, since it is an additional reporting feature. Folks not seeing the suppressed exceptions can still see the stack trace of the main exception. However, I agree that we should try to find a better solution if we can. I just don’t think that it’s a show-stopper if we cannot find a better solution.

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

No branches or pull requests

3 participants