Assertj-core: 'toString' method of subclasses of AtomicReference isn't used for presentation

[chashnikov]

Summary

Sometimes we extend AtomicReference in our classes to store some internal data. (Of course, usually it's better to use delegation instead of inheritance, but it may increase memory footprint and it may be undesirable in some cases.) StandardRepresentation.toStringOf returns special representation for instances of such classes, which doesn't include result of 'toString' call.

Example

Consider the following class

class MyData extends AtomicReference<String> {
  private String description;

  MyData(String description) {
    this.description = description;
  }

  @Override
  public String toString() {
    return "MyData[" + description + "]";
  }
}

If you use its instances in assertions like

    assertThat(new MyData[] {new MyData("foo")}).contains(new MyData("bar"));

you'll get error messages like

Expecting MyData[]:
[AtomicReference[null]]
to contain:
[AtomicReference[null]]
but could not find the following mydata(s):
[AtomicReference[null]]

which aren't helpful.

[chashnikov]

I suggest to use conditions like object.getClass() == AtomicReference.class instead of object instanceof AtomicReference in StandardRepresentation.toStringOf to fix such problems.

[joel-costigliola]

Fair enough!

[Lanninger08]

Hello, I would like to know whether I could work on this issue！:)

[Lanninger08]

In the current implementation, the toString method of all subclasses will be overwritten by the superclass. ~Do we need to refactor the class to make it stricter?~ This seems to be an issue for all subclasses. So do we need to make the class comparison exact, or provide it as an option?

[joel-costigliola]

In the current implementation, the toString method of all subclasses will be overwritten by the superclass. Do we need to refactor the class to make it stricter?

Sorry I don't understand what you are saying, can you clarify?

I suggest to limit the StandardRepresentation.toStringOf fix for AtomicReference only (I'll think about whether to apply a similar fix the other classes).

You could follow these steps:

1. adding a test demonstrating the issue
2. fix StandardRepresentation.toStringOf
3. verify that the test now succeeds

[Annette0127]

@joel-costigliola Hi, according to the description by @chashnikov, I believe @Lanninger08 means that all if(object instanceof SomeParentClassOfTheObject) will cause the same kind of issue while the toString method of the parent-class instead of the sub-class will be invoked by the sentence after.

For a concrete example:

class SuperClass{
    ...
    public String toString(){
        return "some string defined by the super-class";
    }
}

class SubClass{
    ...
    @Override
    public String toString(){
        return "some string defined by the sub-class";
    }
}

If in Representation we check for

(subClassObject instanceof SuperClass)

then the toString of the subclass is never invoked.

[scordio]

I suggest to use conditions like object.getClass() == AtomicReference.class instead of object instanceof AtomicReference in StandardRepresentation.toStringOf to fix such problems.

It would cover your example but we would take away the current representation from any other subtype and there is no guarantee that a subtype will define its own toString.

Are we sure we want to apply this change for all the AtomicReference subtypes?

My feeling is that a custom formatter could be registered to address this issue, something like:

 Assertions.registerFormatterForType(MyData.class, MyData::toString);

[scordio]

(Reposting - deleted by mistake)

On second thought, it's probably enough if we add a !hasOverriddenToString(object) check, like:

https://github.com/assertj/assertj-core/blob/5dc550b80937ab3ff41645de7036e97fa0097f38/src/main/java/org/assertj/core/presentation/StandardRepresentation.java#L242

with some adaptations as we would check it only if it's a subtype of AtomicReference.

This would still provide a default representation for all the subtypes of AtomicReference that don't implement toString.

Another improvement could be displaying the subtype name and not AtomicReference for the default representation, i.e., changing:

https://github.com/assertj/assertj-core/blob/5dc550b80937ab3ff41645de7036e97fa0097f38/src/main/java/org/assertj/core/presentation/StandardRepresentation.java#L482-L484

to

 protected String toStringOf(AtomicReference<?> atomicReference) { 
   return String.format("%s[%s]", atomicReference.getClass().getSimpleName(), toStringOf(atomicReference.get())); 
 } 

Similar considerations should probably be applied to AtomicMarkableReference and AtomicStampedReference (and maybe to other non-final JDK types).

[joel-costigliola]

I agree with @scordio above comment.