Add multiline message builder

[joel-costigliola]

Just an idea for the time being ... see if it is worth it.

[mariuszs]

You mean something like Spock assertions messages?

 Condition not satisfied:

 stack.size() == 2
 |     |      |   
 |     1      false
 [push me]

[joel-costigliola]

My first idea was less ambitious, I would like to use a message builder to build them in consistent way. For the time being the error message is hard coded, and, for example, if I want to change the indentation I will have to change almost every error messages.

But if you have ideas for better error message and how to build them, I'm all ears.

[abalhier]

Hello Joel,

It seems like a complicated refactoring to me. If it must be done, we should try to define what defines an assertion error message. Can it be considered like something with, at most, 4 parts which meets most of our needs :

1. Expecting
2. To be / have / end with / ...
3. But found / was / had ...
4. And not expecting ...

What should be done for assertion error messages which don't fit this model :

1. Change it to fit this model (if possible) ?
2. Add custom methods to handle these cases ?

• Sub-questions : Allow string formats to customize easily messages ? Doesn't this mean less uniformity ?

Some examples of this :

• ShouldBeWithin : super("%nExpecting:%n <%s>%nto be on %s <%s>", actual, fieldDescription, fieldValue);
• ShouldContainExactly : super("%n" + "Actual and expected have the same elements but not in the same order, at index %s actual element was:%n" + " <%s>%n" + "whereas expected element was:%n" + " <%s>%n%s", indexOfDifferentElements, actualElement, expectedElement, comparisonStrategy);
• ShouldNotContainAtIndex : super("%nExpecting:%n <%s>%nnot to contain:%n <%s>%nat index <%s>%n%s", actual, expected, index.value, comparisonStrategy);

We could have BasicErrorMessageFactory taking an AssertionErrorMessageBuilder in its constructor instead of a formatted String with arguments. Its create() methods would call the toString method of the builder :

Also, what about unit tests and their maintenance ? The assertion error messages and their formatting is tested so every change to the formatting will mean that many tests must be fixed. If we go for uniformity many more tests will be broken after a reformatting of error messages.

Other ideas are welcome :)

[dorzey]

@abalhier, did you start working on this? This is something that I'd be interested in helping out with.

As @abalhier suggested we need to be clear on where we want to end up. But I think it would be useful to state what we believe is wrong with the current (MessageFormatter/ErrorMessageFactory) implementation.

I'm not sure the Spock style suggested by @mariuszs would work for assertj. Spock allows only a single assertion per line, whereas assertj's fluent style means you could have:

assertThat(stack).hasSize(2).contains(1, 2);

A Spock style message would make it more difficult to see which criteria had not been met

Lastly, assuming this gets rolled out iteratively, where is the best place to start?

[joel-costigliola]

I'm actually wondering if this can be addressed without too much work as it is not the most important assertj issue.

I in the first place wanted a message builder to handle indentation when displaying a value, in most of the error message indentation is one space, ex:

Expecting values:
 <["Sam", 38]>
in fields:
 <["name", "age"]>
but were:
 <["Frodo", 33]>
in <Frodo 33 years old Hobbit>.
Comparison was performed on fields:
 <["name", "race", "age"]>

I would like two spaces to make the error more readable, ex:

Expecting values:
  <["Sam", 38]>
in fields:
  <["name", "age"]>
but were:
  <["Frodo", 33]>
in <Frodo 33 years old Hobbit>.
Comparison was performed on fields:
  <["name", "race", "age"]>

[abalhier]

@dorzey, No I haven't started anything yet. I was waiting for answers.

@dorzey @joel-costigliola, for this minimal feature, maybe we can just add a method which will do the normalization and apply it to the actual/expected/notExpected arguments. Many tests may have to be fixed. Perhaps they should be refactored to assert on trimmed lines instead of comparing to the whole error message. What do you think ?

[joel-costigliola]

@abalhier sorry for the lack of feedback.

I don't think that error messages tests should change a lot. Building the error message differently should be an implementation detail. I want to keep these tests as they are because they show what a real error message looks like to the end user, it's important to provide nice and helpful error messages.

We should do a POC/spike to see if can create a message builder that can take care of indentation and also try to address #263.

[abalhier]

Ok @joel-costigliola

I may try something next week-end, but if you want to start asap @dorzey, then you can :) There are other issues I can help with.

[dorzey]

@joel-costigliola - I might have time this week to begin this.

If I've understood your idea correctly then rather than BasicErrorMessageFactory taking a String in the constructor it will take a MessageBuilder meaning that all hard coded Strings, such as those in ShouldBeEqualByComparingOnlyGivenFields, can be removed. Is my understanding correct?

Something like: https://github.com/dorzey/assertj-core/commit/652bdc5ac2308a2c00f36ae3c7ac1427d64c616b

[joel-costigliola]

@dorzey at first sight, it looks really good ! can you try migrate different kind of error messages to see if it fits well. Let ShouldBeEqual aside as it is not a simple error message.

[dorzey]

I've migrated the following (see https://github.com/dorzey/assertj-core/commits/message_builder_spike):

• ShouldHaveAtLeastOneElementOfType
• ShouldBeGreaterOrEqual
• ConditionAndGroupGenericParameterTypeShouldBeTheSame

It's already highlighted some inconsistency in spacing and line break usage.

[dorzey]

I'm more than happy to continue this if we think it is heading in the right direction.

[joel-costigliola]

I'm happy you're happy, please continue in this happy direction.

[dorzey]

Happy to :smiley_cat:

[dorzey]

I've not had time to contribute for a while now. Is this still required? Or are there more pressing issues?

[joel-costigliola]

Welcome back ! :)

I need to think about this one, I'm not so sure it is that important. Let's put it on hold for the time being.

[joel-costigliola]

spiking https://github.com/joel-costigliola/assertj-core/issues/683 would be really nice.