Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TEST] add a test for ElasticSearchRecordable #3154

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

sjinks
Copy link
Contributor

@sjinks sjinks commented Nov 20, 2024

Changes

This PR adds a test for opentelemetry::exporter::logs::ElasticSearchRecordable and updates the (disabled) tests for Elasticsearch Logs Exporter to ensure they compile.

The plan is to refactor the ES Recordable to use the ADL Serializer. This will make the code much more readable and maintainable (no need to use nostd::holds_alternative or value.index() + nostd::get() and keep the list of the types up-to-date). That is why we need a test to make sure that the refactoring will not break the things.

For significant contributions please make sure you have completed the following items:

  • CHANGELOG.md updated for non-trivial changes
  • Unit tests have been added
  • Changes in public API reviewed

Copy link

netlify bot commented Nov 20, 2024

Deploy Preview for opentelemetry-cpp-api-docs canceled.

Name Link
🔨 Latest commit b006b8d
🔍 Latest deploy log https://app.netlify.com/sites/opentelemetry-cpp-api-docs/deploys/673d3e007a6e020008624ad4

Copy link

codecov bot commented Nov 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.84%. Comparing base (4d9cc28) to head (b006b8d).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3154      +/-   ##
==========================================
- Coverage   87.86%   87.84%   -0.01%     
==========================================
  Files         195      195              
  Lines        6151     6151              
==========================================
- Hits         5404     5403       -1     
- Misses        747      748       +1     

see 1 file with indirect coverage changes

---- 🚨 Try these New Features:

@sjinks
Copy link
Contributor Author

sjinks commented Nov 20, 2024

Bazel on MacOS and IWYU failed for unrelated to this PR reasons; I cannot restart these jobs.

@sjinks sjinks marked this pull request as ready for review November 20, 2024 10:58
@sjinks sjinks requested a review from a team as a code owner November 20, 2024 10:58
@marcalff marcalff changed the title test: add a test for ElasticSearchRecordable [TEST] add a test for ElasticSearchRecordable Nov 20, 2024
Copy link
Member

@marcalff marcalff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for the fix.

@marcalff
Copy link
Member

Waiting a bit for possible comments, then will merge.

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