Avoid extending assertj Assertions in OpenTelemetryAssertions

[ejona86]

Describe the bug Because OpenTelemetryAssertions extends Assertions, it brings in assertThat(Object) (spelled assertThat(T)) from assertj. With a static import of assertThat, the extends prevents you from mixing OpenTelemetryAssertions with Truth. Since all the methods are static, it is unclear why it even extends Assertions. I didn't see a previous issue where this was discussed.

Removing the extends would be an API breakage, and it appears this class is stable. So a fix might have to be in the form of a new class, which is unfortunate.

Steps to reproduce

import static com.google.common.truth.Truth.assertThat;
import static io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions.assertThat;

...
// both method <T>assertThat(T) in OpenTelemetryAssertions and method assertThat(Object) in Truth match
assertThat(new Object()).isNotNull();

What did you expect to see? Mix assertThat from multiple test libraries sources and have them only work on their specific types.

What did you see instead? Compile error when using a static import.

Test.java:115: error: reference to assertThat is ambiguous
    assertThat(new Object()).isNotNull();
    ^
  both method <T>assertThat(T) in OpenTelemetryAssertions and method assertThat(Object) in Truth match
  where T is a type-variable:
    T extends Object declared in method <T>assertThat(T)
1 error

The workaround is to use a non-static import for OpenTelemetryAssertions and spell out OpenTelemetryAssertions.assertThat explicitly in tests instead.

What version and what artifacts are you using? io.opentelemetry:opentelemetry-sdk-testing:1.36.0. Build environment is Gradle.

Environment Compiler: AdoptOpenJDK 1.8.0_275-b01 OS: Debian unstable

[jkwatson]

Yeah, if you're going to try to use the OTel assertions with something other than assertJ, you'll have to reference via the class. If you want a transparently Truth-compatible version, I think that might be a whole other set of assertion classes, as you mention.

If you want to create a contrib project with the Truth-compatible versions and maintain it, then I'm sure other Truth users would appreciate it!

[ejona86]

@jkwatson, do you know why OpenTelemetryAssertions is even extending Assertions? It doesn't seem to provide any purpose.

[breedx-splk]

@jkwatson, do you know why OpenTelemetryAssertions is even extending Assertions? It doesn't seem to provide any purpose.

It was that way back in 2020 as of the first commit. My guess is that it just made it convenient to just do a wildcard import or something. Similarly, since we use AssertJ heavily across the java otel projects, it makes it much more convenient to import static io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions.assertThat and be able to do both assertThat(span)... and assertThat(someList).isEmpty() in the same code.....whereas if it didn't extend, you could only static import one of those (as you had also pointed out but for Truth).

Feels bad either way. :)

[jkwatson]

Agree with @breedx-splk : I think it was to make wildcard imports bring in everything from Assertions as well, to avoid double-importing.

[SylvainJuge]

This pattern of extending org.assertj.core.api.Assertions is also the recommended way to do it in AssertJ documentation.

[cpovirk]

I think that's their recommendation for classes that will never be imported along with other assertion classes. It can be convenient for assertions for a self-contained project, but it leads to trouble in combination with another common library that also decides to extend Assertions or with another library that defines assertThat methods for common types.

[jkwatson]

Since we can't change this without breaking backward compatibility at this point, I suggest we table this suggestion, and treat it as a lesson-learned.

[breedx-splk]

This might be a hot take 🌶️ , but I think we should be free (freer? more free?) to make breaking changes to things like test utility code, even when the change is breaking, when it results in improved user experience in the long run.

Aside from semver, is there any other place that denotes opentelemetry-sdk-testing as "stable"?

[jkwatson]

This might be a hot take 🌶️ , but I think we should be free (freer? more free?) to make breaking changes to things like test utility code, even when the change is breaking, when it results in improved user experience in the long run.

Aside from semver, is there any other place that denotes opentelemetry-sdk-testing as "stable"?

By our own versioning scheme, it has been marked as stable, since it does not have the -alpha suffix in the version.