RUM-4030 Setup the trace end tests environment for Otel API#1983
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## feature/otel-support #1983 +/- ##
========================================================
- Coverage 60.39% 60.38% -0.02%
========================================================
Files 808 808
Lines 30193 30196 +3
Branches 4947 4947
========================================================
- Hits 18234 18231 -3
- Misses 10747 10749 +2
- Partials 1212 1216 +4 |
a01a6b1 to
a9e72f6
Compare
a9e72f6 to
e2dbd24
Compare
| import org.assertj.core.data.Offset | ||
| import java.util.Locale | ||
|
|
||
| internal class PayloadAssert(actual: JsonObject) : |
There was a problem hiding this comment.
Note
I would rename this class SpanPayloadAssert instead. PayloadAssert feels quite generic and this class seems very opinionated around the Span data model.
| return this | ||
| } | ||
|
|
||
| class ChildAtAssert(actual: JsonObject, private val index: Int) : |
There was a problem hiding this comment.
Note
Same, I'd rename this as SpanAssert or something similar. ChildAt doesn't really make any sense.
| AbstractObjectAssert<ChildAtAssert, JsonObject>(actual, ChildAtAssert::class.java) { | ||
|
|
||
| fun hasTraceId(traceId: String): ChildAtAssert { | ||
| val key = TRACE_ID_KEY_FORMAT.format(Locale.US, index) |
There was a problem hiding this comment.
Important
Instead of generating a key with the index for every assertions, why don't you get the object at "span[$i]" in the hasSpanAtIndexWith() function, and then pass that JsonObject as argument to this method.
Even though it's just tests, it is error prone to have this every where + quite inneficient.
There was a problem hiding this comment.
I was thinking to leverage those koltin extensions under the hood, also had this approach in mind. I will give it a try.
There was a problem hiding this comment.
Indeed creating all these keys with string formats is not efficient, I wanted to keep the same approach as in initial tests but maybe it's too much.
| * Copyright 2016-Present Datadog, Inc. | ||
| */ | ||
|
|
||
| package com.datadog.android.trace.integration.assertj |
There was a problem hiding this comment.
Note
Based on our testing guidelines, this should be in a package named com.datadog.android.trace.integration.tests.assertj
There was a problem hiding this comment.
nope, it is in the right package there, assertj is not part of tests...maybe I should put it there though 🤔
e2dbd24 to
f2f8837
Compare
4 participants
What does this PR do?
A brief description of the change being made with this pull request.
Motivation
What inspired you to submit this pull request?
Additional Notes
Anything else we should know when reviewing?
Review checklist (to be filled by reviewers)