🧪 test: Add unit tests for initObservability#62
Conversation
Adds a new test file for the `initObservability` and `getObservability` singleton functions in the `packages/observability` module. The tests ensure proper initialization of the singleton, handle multiple instantiation attempts, and ensure the getter function behaves correctly if called prior to initialization. Properly utilizes `vi.resetModules()` with dynamic imports to avoid test flakiness between tests since the singleton is stateful. Mocked OTel dependencies to prevent network calls. Co-authored-by: groupthinking <154503486+groupthinking@users.noreply.github.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the reliability of the observability package by introducing robust unit tests for its core initialization and retrieval functions. The new tests ensure that the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
🚅 Deployed to the EventRelay-pr-62 environment in EventRelay
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds valuable unit tests for the observability initialization logic, significantly improving test coverage. The tests are well-structured, particularly with the use of vi.resetModules() to handle the singleton pattern. My review includes a couple of suggestions to enhance the tests by avoiding @ts-ignore and direct access to private properties. This will make the tests more robust and better aligned with the repository's strict typing standards.
| // @ts-ignore - accessing private member for verification | ||
| expect(instance.serviceName).toBe('test-service'); | ||
| // @ts-ignore - accessing private member for verification | ||
| expect(instance.enabled).toBe(false); |
There was a problem hiding this comment.
Using @ts-ignore to access private members for verification makes tests brittle and bypasses type safety. This goes against the 'strict type hinting' principle from the repository style guide.
A better approach is to test the observable behavior. For example, to test the enabled: false case, you could assert that NodeSDK is not instantiated. To test the serviceName, you could assert that trace.getTracer is called with the correct name in a separate test where observability is enabled.
References
- The use of
@ts-ignorebypasses TypeScript's type checking, which is contrary to the principle of strict type hinting mentioned in the repository style guide (line 7). (link)
| // @ts-ignore | ||
| expect(instance2.serviceName).toBe('test-service'); |
There was a problem hiding this comment.
As with the previous comment, it's best to avoid @ts-ignore to maintain strict type safety.
To verify that the singleton instance is not reconfigured on subsequent calls, you could check that the initialization logic (e.g., calls to trace.getTracer()) is only executed once, even when initObservability is called multiple times. This tests the behavior without accessing private implementation details.
References
- The use of
@ts-ignorebypasses TypeScript's type checking, which is contrary to the principle of strict type hinting mentioned in the repository style guide (line 7). (link)
There was a problem hiding this comment.
Pull request overview
Adds unit test coverage for the packages/observability singleton initialization surface (initObservability / getObservability) and wires up the package to run tests with Vitest in the workspace.
Changes:
- Added Vitest unit tests covering initialization, idempotent init behavior, and pre-init access errors.
- Added a
testscript and Vitest dev dependency for@repo/observability(plus TypeScript config dependency alignment). - Updated root
package-lock.jsonto reflect the new dev tooling/dependency resolution.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/observability/src/observability.test.ts | Adds unit tests for initObservability / getObservability behavior using Vitest + module mocking. |
| packages/observability/package.json | Adds vitest run test script and required devDependencies (Vitest, TS config, etc.). |
| package-lock.json | Lockfile updates resulting from adding Vitest/tooling and updated dependency resolution. |
| import { describe, it, expect, beforeEach, vi } from 'vitest'; | ||
|
|
||
| // Mock the open telemetry modules | ||
| vi.mock('@opentelemetry/sdk-node', () => { | ||
| return { |
There was a problem hiding this comment.
This test file is located under src/, and packages/observability/tsconfig.json currently includes all of src without excluding *.test.ts. That means tsc will emit this test into dist/ during npm run build, which is usually undesirable for a library package. Consider moving tests outside src (e.g., tests/), or updating the tsconfig exclude/include patterns to omit test files.
| // @ts-ignore - accessing private member for verification | ||
| expect(instance.serviceName).toBe('test-service'); | ||
| // @ts-ignore - accessing private member for verification | ||
| expect(instance.enabled).toBe(false); |
There was a problem hiding this comment.
These assertions rely on @ts-ignore to access private members (serviceName, enabled). That makes the tests brittle against refactors (e.g., switching to #private fields) and tests implementation details rather than behavior. Prefer asserting observable behavior instead (e.g., verify OpenTelemetry SDK setup methods are/aren’t invoked based on config), or add a minimal read-only getter if you really need to validate config.
The `actions/github-script@v7` runs were failing with `HttpError: Resource not accessible by integration` when attempting to add labels or create comments on Pull Requests. This adds the missing `pull-requests: write` and `issues: write` permissions to the `.github/workflows/auto-label.yml` and `.github/workflows/pr-checks.yml` workflows. Co-authored-by: groupthinking <154503486+groupthinking@users.noreply.github.com>
🔍 PR Validation |
The `actions/github-script@v7` runs were failing with `HttpError: Resource not accessible by integration` when attempting to add labels or create comments on Pull Requests. This adds the missing `pull-requests: write` and `issues: write` permissions to the `.github/workflows/auto-label.yml` and `.github/workflows/pr-checks.yml` workflows. Co-authored-by: groupthinking <154503486+groupthinking@users.noreply.github.com>
🔍 PR Validation |
The `actions/github-script@v7` runs were failing with `HttpError: Resource not accessible by integration` when attempting to add labels or create comments on Pull Requests. This adds the missing `pull-requests: write` and `issues: write` permissions to the `.github/workflows/auto-label.yml` and `.github/workflows/pr-checks.yml` workflows. Co-authored-by: groupthinking <154503486+groupthinking@users.noreply.github.com>
🔍 PR Validation |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -0,0 +1,91 @@ | |||
| import { describe, it, expect, beforeEach, vi, afterEach } from 'vitest'; | |||
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 2 days ago
In general, unused imports should be removed from the import list to avoid confusion and keep the codebase clean. Here, we only need to adjust the vitest import on line 1 to no longer include afterEach.
The best fix without changing functionality is to edit packages/observability/src/observability.test.ts and remove afterEach from the destructuring import from 'vitest'. No other lines or imports need to change, and no new methods or definitions are required. This keeps all currently used testing helpers (describe, it, expect, beforeEach, vi) while eliminating the unused one.
| @@ -1,4 +1,4 @@ | ||
| import { describe, it, expect, beforeEach, vi, afterEach } from 'vitest'; | ||
| import { describe, it, expect, beforeEach, vi } from 'vitest'; | ||
| import * as observabilityModule from './observability'; | ||
| import { Observability } from './observability'; | ||
|
|
| @@ -0,0 +1,91 @@ | |||
| import { describe, it, expect, beforeEach, vi, afterEach } from 'vitest'; | |||
| import * as observabilityModule from './observability'; | |||
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 2 days ago
In general, unused imports should be removed to improve readability and avoid confusion. If the module is only needed for side effects, it should instead be imported without binding it to a name (e.g., import './module';). Here, the module is already imported dynamically inside each test, so the top-level namespace import is redundant.
The best fix without changing existing functionality is to delete the unused import line import * as observabilityModule from './observability'; in packages/observability/src/observability.test.ts. No additional imports or code changes are required because all references to the observability module use await import('./observability') within tests. Removing this line will not alter runtime behavior or test logic.
Concretely: in packages/observability/src/observability.test.ts, remove line 2 containing the unused import. No new methods, imports, or definitions are needed.
| @@ -1,5 +1,4 @@ | ||
| import { describe, it, expect, beforeEach, vi, afterEach } from 'vitest'; | ||
| import * as observabilityModule from './observability'; | ||
| import { Observability } from './observability'; | ||
|
|
||
| // Mock the open telemetry modules |
| @@ -0,0 +1,91 @@ | |||
| import { describe, it, expect, beforeEach, vi, afterEach } from 'vitest'; | |||
| import * as observabilityModule from './observability'; | |||
| import { Observability } from './observability'; | |||
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 2 days ago
In general, unused imports should be removed to improve readability and avoid confusion. Since all uses of the Observability class go through the dynamically imported mod object (e.g., mod.Observability), the direct named import is unnecessary.
The best fix here is to delete line 3 (import { Observability } from './observability';) from packages/observability/src/observability.test.ts. No other code changes are required, because tests already work with mod.Observability and observabilityModule and do not rely on the directly imported Observability symbol. No additional methods, imports, or definitions are needed.
| @@ -1,6 +1,5 @@ | ||
| import { describe, it, expect, beforeEach, vi, afterEach } from 'vitest'; | ||
| import * as observabilityModule from './observability'; | ||
| import { Observability } from './observability'; | ||
|
|
||
| // Mock the open telemetry modules | ||
| vi.mock('@opentelemetry/sdk-node', () => { |
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
🎯 What: The
initObservabilityandgetObservabilityfunctions inpackages/observability/src/observability.tswere lacking test coverage, meaning initialization behavior wasn't being validated.📊 Coverage: The new tests now fully cover:
Observabilitysingleton.initObservabilityis called multiple times.getObservabilityis called before initialization.✨ Result: Test coverage for these initialization functions went from 0% to fully covered. Vitest and TypeScript configuration dependencies were also added to the workspace appropriately, allowing for simple testing via
npm run test --workspaces.PR created automatically by Jules for task 6699426491412081943 started by @groupthinking