Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

String expectation messages alter content #299

Closed
pak3nuh opened this issue Mar 29, 2020 · 9 comments
Closed

String expectation messages alter content #299

pak3nuh opened this issue Mar 29, 2020 · 9 comments

Comments

@pak3nuh
Copy link

pak3nuh commented Mar 29, 2020

On expectation fail, the library issues a message displaying what went wrong.

If the expectation is a String object it will wrap the differences in angle brackets [ ]. This is a cool feature that highlights where the problem is but changes the content of the expectation and the actual value.

The issue is worse if the values already contain angle brackets by themselves, making it harder to spot where the problem is. Bellow is the output of a failing test.

expected:<...id:[[[[Lkotlin.Int?][?]]?]]"> but was:<...id:[[[[Lkotlin.Int?][]?]]?]">
Expected :...id:[[[[Lkotlin.Int?][?]]?]]"
Actual   :...id:[[[[Lkotlin.Int?][]?]]?]"

I've checked the custom fail messages, but it seems a different use case.

It would be nice to allow disabling this highlight feature.

Thanks

@evant
Copy link
Collaborator

evant commented Mar 29, 2020

Open to suggestions, would be worth seeing how other assertion libs handle this.

@BrynCooke
Copy link

In IntelliJ it makes things difficult because it doesn't work nicely with the inbuilt assertion diff feature.
It's not so bad for single lines comparisons, but when comparing a multi line block you definitely feel it.
In comparison JUnit5 assertions just leave the expected and actuals as they are.
It'd be great to be able to disable this feature.

@evant
Copy link
Collaborator

evant commented May 19, 2020

Hm, I'd rather not introduce a configuration for this. However I do think there's room for improvement with the error message. A couple of ideas would be to 1: move the diff to it's own lines so that the original contents are included and it's easier to scan, and 2: change the diff symbol so something that's less likely to be used in the contents. Something like:

org.opentest4j.AssertionFailedError: expected:<"[[[[Lkotlin.Int?]?]?]]"> but was:<"[[[[Lkotlin.Int?]?]]?]">
- [[[[Lkotlin.Int?]?]【?]】]
+ [[[[Lkotlin.Int?]?]【]?】]

Downside of this is intellij's assertion diff feature is extremely picky and won't show in this case. It does show if the additional diff is above instead of below, but I don't think that reads very well. Thoughts?

@BrynCooke
Copy link

BrynCooke commented May 20, 2020

There is a related issue in opentest4j. I guess diff should eventually be solved there.
ota4j-team/opentest4j#59

If it's solved at the opentest4j level then it'll eventually make it in to JUnit5, which in turn will be eventually be supported by all the major IDEs.

@pak3nuh
Copy link
Author

pak3nuh commented May 22, 2020

Hello all,

Sorry about vanishing, but some mails are going to my gmail social folder. I need to update those filters.
Changing the symbol for another less used one I guess will reduce the frequency the issue occurs but won't eliminate it.

@evant Might I ask why don't you want to introduce another configuration?

@BrynCooke Even if it is fixed on opentest4j, the core issue will still be present, since the assertion text will introduce extra characters.
I'm not aware if assertK uses opentest4j somehow. Should we expose this issue for their analysis?

@evant
Copy link
Collaborator

evant commented May 22, 2020

The reason I don't want to introduce a configuration is that I don't think it adequately solves the problem. The way I see it, there's two issues here:

  1. The diff may have readability issues. In this case we should improve it to be more readable instead of turning it off.
  2. The diff breaks intellij's inbuilt assertion diff feature. Imo this is actually a bug in intellij, (they should be using the expected and actual values exposed by opentest4j directly instead of trying to parse the failure message). However this is widely used so if we can not break it that would be ideal. I don't think we should do it at the expense of the error message outside of intellij though. A config flag here would be particularly gross as you'd have to somehow figure out how to set it when running in intellij and not when outside.

Also, not aware of any other assertion lib out there that lets you configure this.

@BrynCooke
Copy link

I don't think there is a good answer at the moment. This requires some sort of fix outside of the assertk codebase. So probably just leave things as they are for now.

@pak3nuh
Copy link
Author

pak3nuh commented May 22, 2020

This is not a critical issue because it should not happen that often.

Although I think that compatibility with Intellij should be an afterthought. After all, most of the time I look at those messages is through a command line.

Feel free to close the issue if you think it should be addressed at another level.

@jschneider
Copy link

These "[]" really bother me.
To be honest: Quite often I copy the actual result and paste it into my unit test. I know...
But now I have to remove the brackets manually...

I think it is the job of the IDE to highlight the differences. Which works really well with JUnit and IntelliJ IDEA. Therefore I prefer to keep the expected/actual value as they are.

@evant evant closed this as completed Apr 21, 2021
@willowtreeapps willowtreeapps locked and limited conversation to collaborators Apr 21, 2021

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Projects
None yet
Development

No branches or pull requests

4 participants