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-node): recursively discover package.json #2206
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,7 +15,8 @@ | |
*/ | ||
|
||
import * as assert from 'assert'; | ||
import { Instrumentation, InstrumentationBase } from '../../src'; | ||
import sinon = require('sinon'); | ||
import { Instrumentation, InstrumentationBase, InstrumentationModuleDefinition } from '../../src'; | ||
|
||
class TestInstrumentation extends InstrumentationBase { | ||
constructor() { | ||
|
@@ -56,4 +57,62 @@ describe('BaseInstrumentation', () => { | |
assert.strictEqual(called, true); | ||
}); | ||
}); | ||
|
||
describe('_onRequire', () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it should not be added here but in node test only. Common is run for browser and for node There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This explains the CI failure. The browser tests won't run on my machine. I'll move these tests. Apologies for being new, appreciate all the help. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need to apology, we are here to help :), in tests you have folders: |
||
it('loads package.json recursively returning 0.0.0 when undiscovered', () => { | ||
const instrumentation = new TestInstrumentation() | ||
// @ts-expect-error access internal property for testing | ||
const versionSpy = sinon.spy(instrumentation, 'getModuleVersion'); | ||
// @ts-expect-error access internal property for testing | ||
const findSpy = sinon.spy(instrumentation, 'findModulePackage'); | ||
// @ts-expect-error access internal property for testing | ||
const loadSpy = sinon.spy(instrumentation, 'loadModulePackage') | ||
|
||
const moduleDefinition = {} as InstrumentationModuleDefinition<unknown> | ||
// @ts-expect-error access internal property for testing | ||
instrumentation._onRequire<unknown>( | ||
moduleDefinition, | ||
{} as unknown, | ||
'test-module', | ||
'/foo/bar/baz' | ||
) | ||
|
||
sinon.assert.calledOnceWithExactly(versionSpy, '/foo/bar/baz') | ||
sinon.assert.calledWith(findSpy, '/foo/bar/baz') | ||
sinon.assert.calledWith(findSpy, '/foo/bar') | ||
sinon.assert.calledWith(findSpy, '/foo') | ||
sinon.assert.calledThrice(loadSpy) | ||
assert.strictEqual(moduleDefinition.moduleVersion, '0.0.0') | ||
}) | ||
|
||
it('loads package.json recursively returning version when discovered', () => { | ||
const instrumentation = new TestInstrumentation() | ||
// @ts-expect-error access internal property for testing | ||
const versionSpy = sinon.spy(instrumentation, 'getModuleVersion'); | ||
// @ts-expect-error access internal property for testing | ||
const findSpy = sinon.spy(instrumentation, 'findModulePackage'); | ||
// @ts-expect-error access internal property for testing | ||
const loadStub = sinon.stub(instrumentation, 'loadModulePackage') | ||
|
||
loadStub.withArgs('/foo/bar').returns({ | ||
name: 'Test Package', | ||
version: '1.0.2' | ||
}) | ||
|
||
const moduleDefinition = {} as InstrumentationModuleDefinition<unknown> | ||
// @ts-expect-error access internal property for testing | ||
instrumentation._onRequire<unknown>( | ||
moduleDefinition, | ||
{} as unknown, | ||
'test-module', | ||
'/foo/bar/baz' | ||
) | ||
|
||
sinon.assert.calledOnceWithExactly(versionSpy, '/foo/bar/baz') | ||
sinon.assert.calledWith(findSpy, '/foo/bar/baz') | ||
sinon.assert.calledWith(findSpy, '/foo/bar') | ||
sinon.assert.calledTwice(loadStub) | ||
assert.strictEqual(moduleDefinition.moduleVersion, '1.0.2') | ||
}) | ||
}) | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't think we should return a invalid version if the version isn't found, i believe
module.moduleVersion
can already be undefinedThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
version
is used further down withisSupported
and ultimatelysemver.satisfies()
. I'm not familiar enough to determine the behaviour if we were to let undefined into these methods.