-
Notifications
You must be signed in to change notification settings - Fork 20
fix: Add integration tests for OpenTelemetry compatibility and remove… #184
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
base: main
Are you sure you want to change the base?
Conversation
b7b0341 to
ab07580
Compare
src/diagnostic.ts
Outdated
| initializeChannels() | ||
| return _producerSendsChannel[prop as keyof typeof _producerSendsChannel] | ||
| } | ||
| }) |
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.
Unfortunately using proxy would slow this library down massively, we should identify a different approach.
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 think removing the circular export should be enough let me double check it
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.
@mcollina I reverted this file. And it's working also.
Signed-off-by: Bernat Mut <bernat.mut@aireuropa.com>
ab07580 to
6e13771
Compare
mcollina
left a comment
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.
lgtm
| 'protocol/index.ts should not contain circular self-reference') | ||
| }) | ||
|
|
||
| it('should use lazy initialization for diagnostic channels', async () => { |
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 think this should be removed as well
| const __filename = fileURLToPath(import.meta.url) | ||
| const __dirname = dirname(__filename) |
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.
These are not needed anymore, use import.meta.dirname and import.meta.filename
| describe('OpenTelemetry Integration', () => { | ||
| it('should import library without hanging when OTEL hook is active', async () => { | ||
| // Create a minimal test script that imports the library | ||
| const { pathToFileURL } = await import('node:url') |
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.
move this import to the top
| ` | ||
|
|
||
| // Write test script to a temporary file | ||
| const { writeFileSync, unlinkSync, mkdtempSync } = await import('node:fs') |
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.
move to the top
|
|
||
| // Write test script to a temporary file | ||
| const { writeFileSync, unlinkSync, mkdtempSync } = await import('node:fs') | ||
| const { tmpdir } = await import('node:os') |
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.
to the top
| } | ||
| }) | ||
|
|
||
| it('should not have circular self-import in protocol/index.ts', async () => { |
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 think this is not a good test
… circular imports
should fix #181