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

[Fixes # 4533] Replaced cpx2 with local test module #5077

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

Conversation

Annosha
Copy link
Contributor

@Annosha Annosha commented Oct 19, 2024

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:

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

@Annosha Annosha requested a review from a team as a code owner October 19, 2024 09:08
Copy link

codecov bot commented Oct 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.27%. Comparing base (be1737f) to head (fe6eb79).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5077   +/-   ##
=======================================
  Coverage   93.27%   93.27%           
=======================================
  Files         317      317           
  Lines        8195     8195           
  Branches     1641     1641           
=======================================
  Hits         7644     7644           
  Misses        551      551           

@Annosha
Copy link
Contributor Author

Annosha commented Oct 19, 2024

@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.

# This is the 1st commit message:

Added local test pkg

# This is the commit message #2:

add type declaration for test-none-core-module

# This is the commit message #3:

Replaced cpx2 with a local module

# This is the commit message open-telemetry#4:

fixed lint errors
@pichlermarc
Copy link
Member

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
Copy link
Member

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.

@Annosha
Copy link
Contributor Author

Annosha commented Oct 22, 2024

@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.

@pichlermarc
Copy link
Member

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 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[instrumentation] replace cpx2 with a local module in tests/fixtures/ exclusively for testing
2 participants