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

styleguide: tests goroutine leaks and naming #4348

Merged
merged 7 commits into from
Jul 24, 2023

Conversation

pellared
Copy link
Member

@pellared pellared commented Jul 21, 2023

  1. Add rules to style guide to have more consistent test names and to increase their stability.

  2. Rename tests that verifies concurrency safety to have ConcurrentSafe as suffix (I think I found most of them).

  3. internal/global: fix goroutine leaks in tests. Fixes Data race in internal/global/internal_logging.go #4264

    Testing:

    $ go test -race -count=1000
    PASS
    ok      go.opentelemetry.io/otel/internal/global        233.443s
    

    Next week, I plan to check if tests in other packages have the same problem

If requested I can split this to separate PR's. However, I think it may be easier to review everything at once as the code changes give context to the new rules introduced in the style guide.

@pellared pellared added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Jul 21, 2023
@pellared pellared changed the title internal/global: Fix goroutine leaks in tests styleguide: tests goroutine leaks and naming Jul 21, 2023
@codecov
Copy link

codecov bot commented Jul 21, 2023

Codecov Report

Merging #4348 (1a2a0dd) into main (cbc5890) will decrease coverage by 0.1%.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #4348     +/-   ##
=======================================
- Coverage   83.4%   83.4%   -0.1%     
=======================================
  Files        184     184             
  Lines      14360   14360             
=======================================
- Hits       11988   11986      -2     
- Misses      2145    2147      +2     
  Partials     227     227             

see 1 file with indirect coverage changes

@pellared pellared merged commit fd5284f into open-telemetry:main Jul 24, 2023
21 checks passed
@pellared pellared deleted the fix-race-internal-tests branch July 24, 2023 07:35
@MrAlias MrAlias added this to the v1.17.0 milestone Aug 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Data race in internal/global/internal_logging.go
4 participants