[TEST] Fix failing W3C Trace Context compliance tests #3115
Conversation
✅ Deploy Preview for opentelemetry-cpp-api-docs canceled.
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3115 +/- ##
==========================================
+ Coverage 87.12% 87.85% +0.73%
==========================================
Files 200 195 -5
Lines 6109 6138 +29
==========================================
+ Hits 5322 5392 +70
+ Misses 787 746 -41
|
psx95
force-pushed
the
w3c-trace-context
branch
9 times, most recently
from
3759a7c
to
39e58f1
Compare
psx95
force-pushed
the
w3c-trace-context
branch
from
39e58f1
to
06283f7
Compare
psx95
force-pushed
the
w3c-trace-context
branch
from
78a34a7
to
ea715cf
Compare
psx95
force-pushed
the
w3c-trace-context
branch
from
ea715cf
to
5d1d476
Compare
Using cin.get() as a means to stop the server listening for requests is preventing the server from being used in a CI environment. This also allows the test server to run in background since exit is now controlled by a http request.
psx95
force-pushed
the
w3c-trace-context
branch
from
151e8f8
to
22b8eb1
Compare
Also explicitly stops the background server serving requests.
psx95
changed the title
psx95
force-pushed
the
w3c-trace-context
branch
from
6cd196d
to
ebaaaf3
Compare
psx95
force-pushed
the
w3c-trace-context
branch
from
716c5fa
to
bb5f91a
Compare
| env: | ||
| CC: /usr/bin/gcc-10 | ||
| CXX: /usr/bin/g++-10 | ||
| PROTOBUF_VERSION: 21.12 |
There was a problem hiding this comment.
Is PROTOBUF_VERSION needed here?
| run: | | ||
| ./ci/do_ci.sh cmake.w3c.trace-context.build-server | ||
| cd $HOME/build/ext/test/w3c_tracecontext_test | ||
| ./w3c_tracecontext_test & |
There was a problem hiding this comment.
Is the tailing & for running the test in background? If so, is it needed?
There was a problem hiding this comment.
It is starting the test web server, so required in background. But in general good to invoke such services under nohup instead of '&', so that it doesn't corrupt the CI terminal output.
ps - just fyi, not action required for my comment.
There was a problem hiding this comment.
Yes, I also just read that in the w3c_tracecontext_test, probably better to imply that is is test server in the file name:)
| -DBUILD_W3CTRACECONTEXT_TEST=ON \ | ||
| -DCMAKE_CXX_STANDARD=${CXX_STANDARD} \ | ||
| "${SRC_DIR}" | ||
| eval "$MAKE_COMMAND" |
There was a problem hiding this comment.
nit, make -j $(nproc) to utilize all available cores to build.
| @@ -15,6 +15,9 @@ Increment the: | |||
|
|
|||
| ## [Unreleased] | |||
|
|
|||
| * [TEST] Fix failing W3C Trace Context compliance tests [#3115](https://github.com/open-telemetry/opentelemetry-cpp/pull/3115) | |||
There was a problem hiding this comment.
[TEST] may mean it only affects test code, but this fixes W3C trace context API?
Fixes #74
Changes
SPEC_LEVEL=1.TraceContextTest&AdvancedTest.Details
The following lists shows all the failing tests. Checked box indicates that the changes in this PR have fixed the test.
Currently failing tests from TraceContextTest suite
Currently failing tests from AdvancedTest suite
NOTE: The tests above were part of test suites
TraceContextTest&AdvancedTest. These test suites Trace Context Level 1 support.Current Status
All tests now passing. Test suites ran -
TraceContextTest&AdvancedTest.Additional Tasks
For significant contributions please make sure you have completed the following items:
CHANGELOG.mdupdated for non-trivial changes