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: Fix simplecov configuration #1366

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

Conversation

zvkemp
Copy link

@zvkemp zvkemp commented Jan 24, 2025

Most of the gemspecs here include simplecov, but the majority did not invoke it. Of the ones that did invoke it, none of them did so at the correct place, so the only coverage tracked was generally for the test helper itself. This commit adds a project-wide simplecov configuration file, requires simplecov in the correct place, and ensures results from different appraisals are merged (rather than clobbered).

@kaylareopelle kaylareopelle changed the title Fix simplecov configuration test: Fix simplecov configuration Jan 24, 2025
@zvkemp zvkemp force-pushed the fix-simplecov branch 6 times, most recently from 48b7ef8 to 5532ddb Compare January 27, 2025 15:38
@arielvalentin
Copy link
Collaborator

Thank you for catching this @zvkemp.

I think one challenge we face here is that we are not sure what to do about our code coverage since we have not defined or enforced a policy around this.

Several of the builds are now failing with your fix in place. Its unclear to me what we should do next.

@open-telemetry/ruby-contrib-approvers Should we lower the threshold for test coverage as part of this PR?

At some point our expectations about code coverage were set to 85%. What is our policy for instrumentations that do not meet this requirement?

@kaylareopelle
Copy link
Contributor

Thanks for opening this PR, @zvkemp!

@open-telemetry/ruby-contrib-approvers Should we lower the threshold for test coverage as part of this PR?

At some point our expectations about code coverage were set to 85%. What is our policy for instrumentations that do not meet this requirement?

A few options come to mind: lower the percentage so that every suite passes, selectively lower coverage (if possible), or exclude certain instrumentation libraries from SimpleCov until they can meet the standard.

Seeing the percentages for the failing suites, I'm don't think lowering the percentage overall is the best bet. I think we'd have to lower things to 50% to get everything to pass, which doesn't seem particularly helpful. Here's the percentages from the last run:
aws_sdk - 84.13%
graphql - 60.34%
kafka - 54.22%
redis - 75.81%

Are we able to lower the coverage in specific instrumentation directories? If so, I propose we lower coverage selectively and open issues to address the coverage problems per instrumentation directory and remove the coverage workaround there. I'm not sure if that's possible. I wonder if an environment variable or an additional .simplecov file local to the offending instrumentation would work. This is my preferred approach to avoid the risk of lowering the coverage rate in the too-low libraries until they have enough coverage.

Otherwise, we could comment out the require 'simplecov' for those libraries with a TODO linked to a comment to try to raise the coverage percentage to 85 to align with the others.

@zvkemp
Copy link
Author

zvkemp commented Jan 27, 2025

Otherwise, we could comment out the require 'simplecov' for those libraries with a TODO linked to a comment to try to raise the coverage percentage to 85 to align with the others.

This sounds fine, though I think I would opt to lower the requirement for these libraries instead. Unfortunately minimum_coverage can be set at most once per process, so it would need to be set in the ENV or a global variable or constant.

I'm currently looking into why graphql et al are reporting low coverage on CI — this does not match what I'm seeing locally. If anything, we should make it not fail until all the results are merged.

@zvkemp
Copy link
Author

zvkemp commented Jan 27, 2025

At least for graphql, it's a matter of the first appraisal having low coverage, so it fails when SimpleCov exits (the merged coverage metric for graphql would have passed). Seems like we should disable the minimum_coverage lint in SimpleCov itself, or use the subprocess mode to handle the merging. Doesn't look like Appraisal has initialization hooks however, so I'm not sure how plausible that is. I can disable the minimum for now.

@zvkemp
Copy link
Author

zvkemp commented Jan 28, 2025

The fix turns out to be fairly minimal — instead of declaring minimum_coverage in the config file, we can just run a dummy coverage job at the end (7ce0306#diff-e018a7f25c2bd16ae50ee7acbd551cb6a4afa64811f45451b454e0896e0d6e0cR171), which includes the minimum_coverage config, and which merges all of the existing results together. None of the 4 libraries that previously failed should have less than 85% now.

@@ -46,6 +46,7 @@ jobs:
ruby: "3.1"
yard: true
rubocop: true
coverage: true
Copy link
Contributor

Choose a reason for hiding this comment

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

Tests for the gems in contrib that aren't specifically instrumentation (propagators, resources, etc.) are run in the .github/workflows/ci-contrib.yml. Could you add coverage: true to that workflow as well?

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