Add AssertionExtensions to work around upstream stability #900
Conversation
…. Migrate NetworkChangeMonitorTest to kotlin.
| fun LogRecordDataAssert.hasEventName(eventName: String): LogRecordDataAssert { | ||
| isNotNull() | ||
| // TODO: This will be removed after the event bits of logs gets out of incubating. | ||
| assertThat(this.actual()).isInstanceOf(ExtendedLogRecordData::class.java) |
There was a problem hiding this comment.
this is likely redundant since the method below will fail if this.actual() as ExtendedLogRecordData fails anyway
| import io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions.assertThat | ||
|
|
||
| fun LogRecordDataAssert.hasEventName(eventName: String): LogRecordDataAssert { | ||
| isNotNull() |
There was a problem hiding this comment.
i think this is redundant since extension LogRecordDataAssert.hasEventName so LogRecordDataAssert is never null, you cant use this extension with log?.hasEventName
There was a problem hiding this comment.
Are we sure about this? The extension is on the LogRecordDataAssert (the assertion class, NOT the log record itself), and the method isNotNull() is on the assetj parent class AbstractAssert and it verifies that the actual result isn't null. I expect that the error message would be easier to read by checking explicitly here, so like assertThat(null).hasEventName("foo") would give a clearer message
Are we talking about the same thing here?
The isInstanceOf() also contains an internal isNotNull() check, but when the eventName comes out of incubating, then the instance checks go away and we will lose the specificity.
|
Given that there are two approvals, I'm going to merge in spite of the unresolved conversation. It's not a big deal to make changes after, but I would like to get the rebase of the following PR going. Thanks for the reviews! |
Given this unfortunate situation with "stability" in the core assertions, let's come up with our own extension(s) for doing assertions. This starts with
hasEventName()on theLogRecordDataAssert.This PR also migrates the
NetworkChangeMonitorTestto kotlin and mockk in order to demonstrate usage of the extension assert.