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 missing tests to Bazel build #3045

Merged
merged 3 commits into from
Sep 3, 2024

Conversation

punya
Copy link
Member

@punya punya commented Sep 1, 2024

Changes

Previously, we were missing

  • instrument_metadata_validator_test
  • observable_registry_test
  • cardinality_limit_test
  • periodic_exporting_metric_reader_test

And there were no checks in place to prevent things from getting worse.

In fact one of these, periodic_exporting_metric_reader_test, fails under ASAN - so we were masking a potential overflow bug.

This issue may exist elsewhere in the repo, so we may want to do a broader audit and/or invest in tooling to make it foolproof.

Copy link

codecov bot commented Sep 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.66%. Comparing base (497eaf4) to head (951faa8).
Report is 123 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3045      +/-   ##
==========================================
+ Coverage   87.12%   87.66%   +0.55%     
==========================================
  Files         200      190      -10     
  Lines        6109     5866     -243     
==========================================
- Hits         5322     5142     -180     
+ Misses        787      724      -63     

see 124 files with indirect coverage changes

@punya punya force-pushed the missing-bazel-tests branch 2 times, most recently from 0def936 to 6fbd858 Compare September 1, 2024 17:41
@punya punya marked this pull request as ready for review September 1, 2024 17:43
@punya punya requested a review from a team September 1, 2024 17:43
@punya punya force-pushed the missing-bazel-tests branch 2 times, most recently from dfc5a5c to 48d6324 Compare September 1, 2024 19:45
@punya punya marked this pull request as draft September 1, 2024 19:47
Also use steady clock consistently.

Prior to this change, the test would fail under ASAN:

bazel test --config=asan --test_output=errors //sdk/test/metrics:meter_provider_sdk_test
INFO: Analyzed target //sdk/test/metrics:meter_provider_sdk_test (0 packages loaded, 0 targets configured).
FAIL: //sdk/test/metrics:meter_provider_sdk_test (see /private/var/tmp/_bazel_punya/e3bd968ba61238cdeb1a5537fc3dbf7d/execroot/_main/bazel-out/darwin_arm64-fastbuild-asan/testlogs/sdk/test/metrics/meter_provider_sdk_test/test.log)
INFO: From Testing //sdk/test/metrics:meter_provider_sdk_test:
==================== Test output for //sdk/test/metrics:meter_provider_sdk_test:
Running main() from gmock_main.cc
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from MeterProvider
[ RUN      ] MeterProvider.GetMeter
[Warning] File: sdk/src/metrics/meter_provider.cc:65 [MeterProvider::GetMeter] Library name is empty.
[Warning] File: sdk/src/metrics/meter_provider.cc:65 [MeterProvider::GetMeter] Library name is empty.
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/c++/v1/__chrono/duration.h:102:59: runtime error: signed integer overflow: 9221646818050376183 * 1000 cannot be represented in type '_Ct' (aka 'long long')
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/c++/v1/__chrono/duration.h:102:59 in 
================================================================================
INFO: Found 1 test target...
Target //sdk/test/metrics:meter_provider_sdk_test up-to-date:
  bazel-bin/sdk/test/metrics/meter_provider_sdk_test
INFO: Elapsed time: 2.251s, Critical Path: 2.13s
INFO: 5 processes: 5 darwin-sandbox.
INFO: Build completed, 1 test FAILED, 5 total actions
//sdk/test/metrics:meter_provider_sdk_test                               FAILED in 0.6s
  /private/var/tmp/_bazel_punya/e3bd968ba61238cdeb1a5537fc3dbf7d/execroot/_main/bazel-out/darwin_arm64-fastbuild-asan/testlogs/sdk/test/metrics/meter_provider_sdk_test/test.log

Executed 1 out of 1 test: 1 fails locally.

Fix overflow in periodic_exporting_metric_reader
Previously, we were missing
* instrument_metadata_validator_test
* observable_registry_test
* cardinality_limit_test
* periodic_exporting_metric_reader_test
And there were no checks in place to prevent things from getting worse.

Remove unnecessary exception checks in attributes_hashmap_test,
which simplifies the build and CI script.

Resolve symbol collision using anonymous namespaces.
@punya punya marked this pull request as ready for review September 1, 2024 22:52
@marcalff marcalff changed the title Add missing tests to Bazel build [TEST] Add missing tests to Bazel build Sep 3, 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.

Well spotted, and thanks for the fix.

@marcalff marcalff merged commit 7f785b5 into open-telemetry:main Sep 3, 2024
52 checks passed
@punya punya deleted the missing-bazel-tests branch September 3, 2024 20:29
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