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

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

Open
3 tasks
pichlermarc opened this issue Mar 8, 2024 · 5 comments · May be fixed by #5077
Open
3 tasks
Assignees
Labels
enhancement New feature or request needs:code-contribution This feature/bug is ready to implement never-stale pkg:instrumentation

Comments

@pichlermarc
Copy link
Member

pichlermarc commented Mar 8, 2024

In @opentelemetry/instrumentation we test RequireInTheMiddleSingleton by using the cpx2 package.

const onRequireCpxStub = makeOnRequiresStub('cpx');
const onRequireCpxLibStub = makeOnRequiresStub('cpx-lib');

As we don't use the cpx2 package anywhere else anymore, having a local test package would help us get rid of that dependency

To complete this issue we need to:

  • add a local package that mimics how cpx2 is structured for testing in ./tests/fixtures/
  • update the tests to use that package instead
  • remove the 'cpx2' devDependency from package.json

It looks like cpx2 is only used as a test package for RequireInTheMiddleSingleton, so picking some other module, or manually creating a local module in a fixtures/ subdir and using that directly would work as well. However, using cpx2 seems fine.

Originally posted by @trentm in #4510 (review)

Copy link

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

@Annosha
Copy link
Contributor

Annosha commented Oct 13, 2024

@pichlermarc To began working on this I want to make sure to move in right direction.
"add a local package that mimics how cpx2 is structured for testing in ./tests/fixtures/" For this purpose I have to create a local module as a test fixture? A fake version of cpx2 inside ./tests/fixtures/.

Question: Do we prefer a particular JS testing framework for opentelemetry-js?

@pichlermarc
Copy link
Member Author

@pichlermarc To began working on this I want to make sure to move in right direction.
"add a local package that mimics how cpx2 is structured for testing in ./tests/fixtures/" For this purpose I have to create a local module as a test fixture? A fake version of cpx2 inside ./tests/fixtures/.

Yes, it looks like I made a mistake when I was writing this - it should not go into ./tests/fixtures/ but rather into .test/node/node_modules. You'll see that there are already two packages in there. You'd add a fake module there. 🙂

That can be a simple commonjs package named something like test-non-core-module that has a lib directory, with an index.js that defines some module.exports similar to how cpx2 is structured (see https://github.com/bcomnes/cpx2/blob/b487cce0b30b51a28647dfdf4757df14b70043b9/lib/index.js). You can just have one export there and have it require another file (which takes the place of copy-sync) that just defines a function as its module.exports. The actual code of the function there can even be a no-op. What we're testing here is just the hooks that should be called when require is called for a package that's being instrumented.

Then you can change the test setup and test to use the new fake test-non-core-module package instead of cpx2.

Question: Do we prefer a particular JS testing framework for opentelemetry-js?

We do use mocha and usually don't accept any changes that start using another framework - so that should stay the same. The goal of this issue is to change the existing tests, there's no need to introduce any new ones 🙂

@Annosha
Copy link
Contributor

Annosha commented Oct 18, 2024

@pichlermarc Please reviewing my local changes, the code is compling without errors or warnings but, @opentelemetry/instrumentation:test failing with AssertError: expected stub to be called with exact arguments
Normalized the paths for discrepancies between win and linux based paths differences.
Please check my changes here for your feedback..
Test log is attached below.
log-error.txt
TIA!

@pichlermarc
Copy link
Member Author

The test-module that was introduced on that branch does not match the way the layout is in cpx2.

In cpx2 the file structure is:

package.json
lib/
 |- index.js
 |- copy-sync.js

and in package.json there's an entry for main which points to lib/index.js. So when require.resolve() will return <full-path>/cpx2/lib which is why the path is then resovled with '..' to get the basePath of the package.

In test-non-core-module the file structure is:

index.js
lib/
 |- copy-sync.js

To mimic the same behavior you can simply move the index.js file to lib/ and adapt the require there to point towards the local directory.

Then you can also add a package.json like so

{
  "name": "test-non-core-module",
  "version": "0.0.1",
  "description": "Local test module for require-in-the-middle singleton",
  "main": "lib/index.js"
}

(the important thing here is main which points it to index.js in the lib directory, which then makes the path.resovle() in the test make sense again 🙂)

@Annosha Annosha linked a pull request Oct 19, 2024 that will close this issue
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request needs:code-contribution This feature/bug is ready to implement never-stale pkg:instrumentation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants