Skip to content

Conversation

@jaydeluca
Copy link
Member

While working on metadata for couchbase, I found that some of the assertions weren't working correctly. These assertions should have actually failed because the test suite wasn't setting the flag to enable experimental attributes.

In this example, it doesn't work as expected because val is an StringAssert object and is never null, so the test passes:

  @Test
  void test() {
    testing().runWithSpan("test", () -> {});

    testing().waitAndAssertTraces(
        traceAssert -> traceAssert.hasSpansSatisfyingExactly(
            span -> span.hasName("test")
                .hasAttributesSatisfyingExactly(
                    satisfies(
                        stringKey("couchbase.local.address"),
                        val -> assertThat(val).isNotNull()))));
  }

You would need to use val -> assertThat(val.actual()).isNotNull()) to get it to fail as expected, or more simply we can use AbstractAssert::isNotNull, like in:

  @Test
  void test() {
    testing().runWithSpan("test", () -> {});

    testing().waitAndAssertTraces(
        traceAssert -> traceAssert.hasSpansSatisfyingExactly(
            span -> span.hasName("test")
                .hasAttributesSatisfyingExactly(
                    satisfies(
                        stringKey("couchbase.local.address"),
                        AbstractAssert::isNotNull))));
  }

This PR cleans up all usages except couchbase, because the couchbase ones require a bit more of a refactor, so that will come in a followup.

@jaydeluca jaydeluca requested a review from a team as a code owner August 19, 2025 10:28
@laurit
Copy link
Contributor

laurit commented Aug 19, 2025

You would need to use val -> assertThat(val.actual()).isNotNull()) to get it to fail as expected, or more simply we can use AbstractAssert::isNotNull

AbstractAssert::isNotNull is same as val -> val.isNotNull()

@jaydeluca jaydeluca mentioned this pull request Aug 19, 2025
@trask
Copy link
Member

trask commented Aug 19, 2025

val -> val.isNotNull()

this reads a bit better to me, @jaydeluca wdyt?

@jaydeluca
Copy link
Member Author

val -> val.isNotNull()
this reads a bit better to me, @jaydeluca wdyt?

Intellij peer pressured me into the shorthand. i do think the other way reads better, i'll update it @trask

@trask trask enabled auto-merge (squash) August 19, 2025 15:28
@trask trask merged commit a549527 into open-telemetry:main Aug 19, 2025
89 checks passed
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

Successfully merging this pull request may close these issues.

3 participants