opentelemetry-js icon indicating copy to clipboard operation
opentelemetry-js copied to clipboard

[Fixes # 4533] Replaced cpx2 with local test module

Open Annosha opened this issue 1 year ago • 4 comments

Which problem is this PR solving?

Replaced the external dependency of cpx2 package, with a local test module (test-non-core-module) that mimics the cpx2 package. Made the required changes in RequireInTheMiddleSingleton.

Fixes #4533

Short description of the changes

Added the module test-non-core-module with package.json. Updated RequireInTheMiddleSingleton.test.ts Also updated package.json to remove the cpx2 dependency.

Changelog Entry

This change does not require a changelog entry as it only modifies internal logic without impacting public APIs.

How Has This Been Tested?

npm run compile npm test with no errors or warnings

Checklist:

  • [x] Followed the style guidelines of this project
  • [ ] Unit tests have been added
  • [ ] Documentation has been updated

Annosha avatar Oct 19 '24 09:10 Annosha

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 93.17%. Comparing base (eb3ca4f) to head (35f1ad2). Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5077      +/-   ##
==========================================
- Coverage   93.18%   93.17%   -0.02%     
==========================================
  Files         315      315              
  Lines        8086     8086              
  Branches     1617     1617              
==========================================
- Hits         7535     7534       -1     
- Misses        551      552       +1     

see 1 file with indirect coverage changes

codecov[bot] avatar Oct 19 '24 09:10 codecov[bot]

@pichlermarc Please review my changes.

Changelog and unit test are failing: For changelog I added a comment in PR specifying changelog isn't required. Why are the Unit/ browser tests failing? The tests and compile were running without errors on my local machine.

Annosha avatar Oct 19 '24 11:10 Annosha

there's a massive amount of changes in ./package-lock.json due to changes in the top-level ./package.json (my guess is some added dependency makes the test fail). Please undo the changes to both files and re-install dependencies npm install --package-lock-only - that should get you the "old" state while still removing the dependency to cpx2.

pichlermarc avatar Oct 21 '24 08:10 pichlermarc

WRT to the changelog, usually someone with Triage permissions decides if the entry needs a changelog or not by applying the Skip Changelog label it it's not necessary. I added it just now.

pichlermarc avatar Oct 21 '24 08:10 pichlermarc

@pichlermarc restored both files. There could be couple reasons for these new dependencies. What is the most likely reason here? And as a developer how is one supposed to know that these are unnecessary changes?

I see this message on this PR too. c

Note: TY for your patience and guidance.

Annosha avatar Oct 22 '24 09:10 Annosha

There could be couple reasons for these new dependencies. What is the most likely reason here? And as a developer how is one supposed to know that these are unnecessary changes?

The most likely reason the diff in https://github.com/open-telemetry/opentelemetry-js/pull/5077/files/fe6eb7917fd994c88cb4e6280dc04ef4b120b072#diff-053150b640a7ce75eff69d1a22cae7f0f94ad64ce9a855db544dda0929316519 is that you might've run something like npm install sinon and npm install require-in-the-middle at some point during the development process. This also updates the package-lock.json and introduces some problems (in this case a new major version of sinon that was not compatible with our current webpack config - breaking changes in new major versions are to be expected).

I see this message on this PR too.

A large part of our contributions never touch any dependencies so this PR was a bit of a special case. The remaining conflicts with the package-lock.json are the product of a trade-off we have to make in this repo to keep our our test workflows somewhat stable:

Transitive dependencies often change and we use a lot of tooling to test against Node.js and in a browser-like environment. We've made the experience that just pinning the devDependencies is not sufficient, as these transitive dependencies update and cause intermittent failures in CI. This was very difficult to troubleshoot for us as we didn't have a reference which transitive dependency was causing the issue.

We made the decision to also check-in a package-lock.json to avoid these transitive dependencies from updating, and we now use renovate-bot to do an update of this file on a weekly basis. With the diff in the package-lock.json maintainers can see what transitive dependencies changed and address any issues without holding back others that may be working on different things.

But adding the package-lock.json has the trade-off that there's a larger potential for merge conflicts as you see in this PR right now. When another PR that changes that file is merged, and there's another PR that modifies that file (like this one), we end up having a merge-conflict. The fix for it is relatively simple though (this is how I do it, but there are many other ways):

  • merging main into your branch
  • accepting the changes frommain for package-lock.json
  • running npm install --package-lock-only again, committing, and pushing the result

That will resolve the conflicts and make it so that we can merge the PR :slightly_smiling_face:

pichlermarc avatar Oct 23 '24 09:10 pichlermarc

@pichlermarc Implemented the steps you mentioned. One unit test/browser test is currently failing; could this be related to conflicts in the package-lock.json file? Any advice would be appreciated.

Annosha avatar Oct 25 '24 05:10 Annosha

@pichlermarc I assume this PR is ready to be merged. Can you check please? TIA!

Annosha avatar Nov 02 '24 15:11 Annosha