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

fix(instrumentation-mysql): fix test for mysql2 v3 (#2168) #2451

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

Conversation

tpraxl
Copy link

@tpraxl tpraxl commented Sep 24, 2024

Which problem is this PR solving?

Fixes #2168 - migrate to v3 in tests

Short description of the changes

Update mysql2 dependency to latest version (@3.11.3) and fix tests. Problems during npm run compile were fixed by removing an unsafe and incorrect typecast.

Update mysql2 dependency to latest version (@3.11.3) and fix
tests. Problems during `npm run compile` were fixed by
removing an unsafe and incorrect typecast.

Fixes open-telemetry#2168

Signed-off-by: Thomas Praxl <[email protected]>
@tpraxl tpraxl requested a review from a team as a code owner September 24, 2024 19:41
Copy link

linux-foundation-easycla bot commented Sep 24, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@tpraxl
Copy link
Author

tpraxl commented Sep 24, 2024

Hold on. I was mislead. While the mysql2 tests do compile now and they signaled success during npm test, all of them were actually skipped (see process.env.RUN_MYSQL_TESTS_LOCAL and process.env.RUN_MYSQL_TESTS).

I really think you should generally separate skipped tests from succeeded tests in the output.

I only noticed that, because I actually want to fix #1511 and started to work on that.

Let's see if I can fix the tests when they are not skipped. Might take a while, depending on the time I can spare. In the meantime: Should I close this PR or do you want to keep it open?

@tpraxl
Copy link
Author

tpraxl commented Sep 26, 2024

The tests succeeded (except for fastify and mongoose), once I figured out that I need to export RUN_MYSQL_TESTS="true" in order for otherwise skipped tests not to signal that they succeeded, AND have a docker container running with exact these arguments:

podman run --rm -d --name otel-mysql -p 33306:3306 -e MYSQL_ROOT_PASSWORD=rootpw -e MYSQL_DATABASE=test_db -e MYSQL_USER=otel -e MYSQL_PASSWORD=secret mysql:5.7 --log_output=TABLE --general_log=ON

(Where general_log=ON is necessary for some tests that failed before)

You should mention that somewhere in the README under "How to run tests the right way".

This PR is now ready for review.

Copy link

codecov bot commented Sep 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.77%. Comparing base (dfb2dff) to head (43da503).
Report is 247 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2451      +/-   ##
==========================================
- Coverage   90.97%   90.77%   -0.21%     
==========================================
  Files         146      156      +10     
  Lines        7492     7716     +224     
  Branches     1502     1584      +82     
==========================================
+ Hits         6816     7004     +188     
- Misses        676      712      +36     

see 76 files with indirect coverage changes

Copy link
Contributor

@david-luna david-luna 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 contributing to OpenTelemetry :)

@haddasbronfman any feedback?

@github-actions github-actions bot added the pkg-status:unmaintained This package is unmaintained. Only bugfixes may be acceped until a new owner has been found. label Oct 11, 2024
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.

@tpraxl
Copy link
Author

tpraxl commented Oct 12, 2024

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.

Uh.. can anyone explain what happened? How likely is this PR being closed in 14 days?

@maryliag
Copy link
Contributor

@tpraxl this message is automatic for all unmaintained packages, to make it clear we won't accept new features unless there is a new code owner for it, but we're still accepting fixes, so we would be able to merge your PR, is just a matter of getting the approval and a maintainer merging it.
In case it doesn't happen and it gets closed 14 days, you can just re-open and poke someone with the permissions to merge this for you :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:instrumentation-mysql2 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.

mysql2 - migrate to v3 in tests
4 participants