Skip to content

Conversation

@onurtemizkan
Copy link
Member

Which problem is this PR solving?

  • Test coverage of the MongoDB instrumentation is currently 49.74% - Link

This is because tests for MongoDB versions v3 and v4 are not included in the coverage report.

Short description of the changes

  • Runs v3 and v4 tests in the test command to make sure they are included in the coverage report

@github-actions github-actions bot added pkg:instrumentation-mongodb pkg-status:unmaintained This package is unmaintained. Only bugfixes may be acceped until a new owner has been found. labels Jan 27, 2025
@codecov
Copy link

codecov bot commented Jan 27, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.41%. Comparing base (92106ff) to head (d71d793).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2681      +/-   ##
==========================================
+ Coverage   90.96%   92.41%   +1.45%     
==========================================
  Files         171      171              
  Lines        8133     8133              
  Branches     1649     1649              
==========================================
+ Hits         7398     7516     +118     
+ Misses        735      617     -118     

see 1 file with indirect coverage changes

@onurtemizkan onurtemizkan marked this pull request as ready for review January 27, 2025 18:25
@onurtemizkan onurtemizkan requested a review from a team as a code owner January 27, 2025 18:25
@github-actions
Copy link
Contributor

This package does not have an assigned component owner and is considered unmaintained. As such this package is in feature-freeze and this PR will be closed with 14 days unless a new owner or a sponsor (a member of @open-telemetry/javascript-approvers) for the feature is found. It is the responsibility of the author to find a sponsor for this feature.
Are you familiar with this package? Consider becoming a component owner.

"test": "npm run test-v3 && npm run test-v4 && npm run test-v5-v6 && nyc merge .nyc_output ./coverage/coverage-final.json",
"test-v3": "tav mongodb 3.7.4 npm run test-v3-run",
"test-v4": "tav mongodb 4.17.0 npm run test-v4-run",
"test-v5-v6": "tav mongodb 6.8.0 npm run test-v5-v6-run",
Copy link
Contributor

Choose a reason for hiding this comment

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

do you think is necessary to pin here the version? pacakge.json file already has the version fixed and if we want to update to a newer version we need to do it in both places.

Copy link
Member Author

Choose a reason for hiding this comment

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

I just double checked, for the pinned version, it's not needed. 👍

@david-luna
Copy link
Contributor

david-luna commented Jan 29, 2025

I have my doubts about this PR. IMHO the wider the support range is the more tests with different versions we would need to do in order to aim for high coverage. We may end up doing a subset of test-all-versions 🤔

I'll bring up this into today's SIG

@onurtemizkan
Copy link
Member Author

Thanks for the review @david-luna!

The alternative I thought was generating coverage on test-all-versions and merging reports in the end. But that seems a bit wider scale update if that's going to be applied to every package (there are just a few packages excluding specific versions as far as I can see).

@david-luna
Copy link
Contributor

david-luna commented Jan 29, 2025

Thanks for the review @david-luna!

The alternative I thought was generating coverage on test-all-versions and merging reports in the end. But that seems a bit wider scale update if that's going to be applied to every package (there are just a few packages excluding specific versions as far as I can see).

@onurtemizkan we discussed this approach for coverage in the SIG. We accept the tradeoff of running 3 different tests to get the coverage report. Also

  • locally you can get a different version of the package under test if npm run test fails in one of the steps.
  • we will keep an eye on the CI to be sure is not impacting it significantly

Thanks for your contribution :) I'm going to approve, make sure your branch is up to date in order to be merged.

@david-luna
Copy link
Contributor

@onurtemizkan sorry for the delay on this PR. I was checking if it's possible to have the coverage in TAV tests. Instead of stalling this PR I'll try it myself in another instrumentation.

@david-luna david-luna merged commit bc349a2 into open-telemetry:main Feb 10, 2025
25 checks passed
deejay1 pushed a commit to deejay1/opentelemetry-js-contrib that referenced this pull request Apr 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pkg:instrumentation-mongodb pkg-status:unmaintained:autoclose-scheduled pkg-status:unmaintained This package is unmaintained. Only bugfixes may be acceped until a new owner has been found.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants