Feature/unit tests for integrations#620
Conversation
…ts for ticket and field mappers
… feature/unit-tests-for-integrations
…Service and articles mapper
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
WalkthroughThis PR adds Vitest-based test suites across many integrations, per-package Vitest configs, coverage collection/merge/report tooling and CI steps, enables Vitest coverage reporting, updates several package.json test/devDependencies, and adds four .gitignore entries and small code/test tweaks. Changes
Sequence Diagram(s)(omitted — changes are primarily tests, config, and CI steps) Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Coverage Report for packages/configs/vitest-config
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
…age collection script
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @.github/actions/test/action.yaml:
- Around line 80-88: The workflow step using
davelosert/vitest-coverage-report-action@v2 includes unsupported inputs; remove
the unsupported keys "working-directory" and "vite-config-path" from the Vitest
coverage step and keep only the supported parameters (e.g., "json-summary-path",
"json-final-path", and "github-token") in the step configuration; if you must
adjust working directory or Vite config, consult the action's action.yml/README
for the correct option names or move that configuration into the Vitest config
file instead.
In `@packages/configs/vitest-config/package.json`:
- Around line 10-22: Add tsx to this package's devDependencies and make glob
versioning consistent with the other devDependencies: in package.json ensure the
"collect-json-reports" script (uses tsx) has a corresponding devDependency entry
for "tsx" and change "glob": "13.0.0" to use caret range "glob": "^13.0.0" so it
matches the caret style used by nyc/open-cli/vitest; update the devDependencies
block accordingly.
In `@packages/configs/vitest-config/turbo.json`:
- Around line 12-16: The "report" task's inputs path is incorrect: update the
"report" task (the report object) to use the merged coverage glob that matches
the "merge-json-reports" output (change inputs from "coverage/merge" to
"coverage/merged/**") so the report task correctly consumes the merged coverage
files.
🧹 Nitpick comments (3)
packages/configs/vitest-config/scripts/collect-json-outputs.mjs (2)
87-92: Incomplete multi-character sanitization (CodeQL finding).The
replaceDotPatternsfunction only performs a single-pass replacement. Input like....//would become../after replacement. While this is only used for console logging (not file operations) and poses no security risk here, it could produce confusing output.🔧 Proposed fix for complete sanitization
- const replaceDotPatterns = (str) => str.replace(/\.\.\//g, ''); + const replaceDotPatterns = (str) => { + let result = str; + while (result.includes('../')) { + result = result.replace(/\.\.\//g, ''); + } + return result; + };
96-98: Add error handling for JSON parsing.If a
coverage-summary.jsonfile is malformed,JSON.parsewill throw and be caught by the outer try-catch, causing the entire collection to fail. Consider handling parse errors per-file to allow partial success.🔧 Proposed fix with per-file error handling
if (summaryFiles.length > 0) { const summaries = await Promise.all( - summaryFiles.map((p) => fs.readFile(p, 'utf8').then(JSON.parse)), + summaryFiles.map((p) => + fs.readFile(p, 'utf8') + .then(JSON.parse) + .catch((err) => { + console.warn(`Warning: Failed to parse ${p}: ${err.message}`); + return null; + }) + ), ); - const merged = { total: aggregateTotals(summaries) }; + const validSummaries = summaries.filter(Boolean); + const merged = { total: aggregateTotals(validSummaries) };.github/actions/test/action.yaml (1)
48-50: Consider adding error handling for the collect step.If
collect-json-reportsfails (e.g., no coverage data exists yet), the workflow will fail before the coverage check can determine whether to proceed. Addingcontinue-on-error: truewould allow the subsequent check step to gracefully handle the absence of coverage.♻️ Proposed fix
- name: Collect coverage shell: bash + continue-on-error: true run: npx turbo run collect-json-reports --filter=@o2s/vitest-config
… Medusa integration services
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@packages/integrations/medusajs/src/modules/resources/resources.mapper.spec.ts`:
- Around line 106-121: The test reveals mapServices returns the filtered array
length as total; update mapServices so its returned total uses the original
server-side count (data.count) instead of services.length to match mapAssets
behavior; locate the mapServices function and replace the total assignment that
references services.length with data.count so pagination reflects the server
total even when items are filtered.
🧹 Nitpick comments (4)
packages/integrations/medusajs/src/modules/resources/resources.service.spec.ts (2)
57-57: Consider usingvi.clearAllMocks()instead ofvi.restoreAllMocks().
vi.restoreAllMocks()is intended for restoring spied implementations to their original state. Since this test usesvi.fn()mocks rather thanvi.spyOn(),vi.clearAllMocks()would be more semantically appropriate for resetting mock call history between tests.Suggested change
beforeEach(() => { - vi.restoreAllMocks(); + vi.clearAllMocks(); mockHttpClient = { get: vi.fn() };
81-91: Standardize error messages in service implementation.The error messages thrown by
purchaseOrActivateResource(line 45:'Method not implemented') andpurchaseOrActivateService(line 49:'Method not implemented.') are inconsistent—one lacks a trailing period while the other includes one. The tests correctly match these implementations, but the service methods should use consistent error messaging. Choose a single format and apply it to both methods.packages/integrations/medusajs/src/modules/products/products.service.spec.ts (1)
113-120: Consider movingthrowErrorimport to file level.The dynamic import of
throwErrorinside the test is functional but unconventional. Moving it to the top-level imports alongsideofwould be cleaner.♻️ Suggested refactor
-import { firstValueFrom, of } from 'rxjs'; +import { firstValueFrom, of, throwError } from 'rxjs';Then simplify the test:
it('should throw NotFoundException when HTTP returns 404', async () => { - const { throwError } = await import('rxjs'); mockHttpClient.get.mockReturnValue(throwError(() => ({ status: 404 })));packages/integrations/medusajs/src/modules/medusajs/medusajs.service.spec.ts (1)
9-13: Simplify the mock implementation.The
this: unknowntype annotation andfunctionsyntax are unnecessary since the mock simply returnsmockSdkwithout usingthis.♻️ Suggested simplification
vi.mock('@medusajs/js-sdk', () => ({ - default: vi.fn().mockImplementation(function (this: unknown) { - return mockSdk; - }), + default: vi.fn(() => mockSdk), }));
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/configs/vitest-config/scripts/collect-json-outputs.mjs`:
- Around line 40-53: The current use of path.basename(match) (assigned to
directoryName) causes collisions when different packages/apps share the same
directory name; change the naming to a unique identifier by computing the
relative path from the repository root (e.g., path.relative(process.cwd(),
match)) and normalize it into a safe filename (replace path separators with
underscores or URL-encode) and use that value instead of directoryName when
building destinationFile and summaryDestinationFile so copies to rawFinalDir and
rawSummaryDir cannot overwrite each other; update all references that create
`${directoryName}.json` (the coverageFilePath copy and the COVERAGE_SUMMARY_PATH
handling) to use the new unique name.
…com/o2sdev/openselfservice into feature/unit-tests-for-integrations
…nd Strapi integrations
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/integrations/strapi-cms/src/modules/graphql/graphql.service.ts`:
- Line 21: The import at the end of
packages/integrations/strapi-cms/src/modules/graphql/graphql.service.ts was
changed to a relative path; revert it to the project alias used everywhere else
by importing from "@/generated/strapi" instead of the relative
'../../../generated/strapi' so graphql.service.ts matches the other files (e.g.,
cms.table.mapper.ts, cms.service.ts) and stays consistent with the codebase's
alias resolution.
🧹 Nitpick comments (2)
packages/integrations/contentful-cms/src/modules/rest-delivery/delivery.service.spec.ts (1)
103-111: Consider a more appropriate type assertion thanas never.The
as nevercast is unconventional here sincenevertypically represents an impossible type. A clearer approach would be to use an explicit partial type oras any.♻️ Suggested improvement
it('should return empty array when items is undefined', async () => { - mockClient.getLocales.mockResolvedValue({} as never); + mockClient.getLocales.mockResolvedValue({} as { items?: undefined }); const service = new RestDeliveryService(mockConfig as unknown as ConfigService);packages/integrations/contentful-cms/src/modules/graphql/graphql.service.spec.ts (1)
32-59: Consider usingvi.clearAllMocks()instead ofvi.restoreAllMocks().
vi.restoreAllMocks()restores mocks to their original implementations, which can interfere with module-levelvi.mock()calls. While the current code works becausemockReset()andmockImplementationOnce()are called afterward, usingvi.clearAllMocks()would be clearer since it only clears call history without affecting the mock implementations.♻️ Suggested change
beforeEach(() => { - vi.restoreAllMocks(); + vi.clearAllMocks(); process.env = { ...originalEnv };
… feature/unit-tests-for-integrations
… feature/unit-tests-for-integrations
3 participants
What does this PR do?
Key Changes
Summary by CodeRabbit
Tests
Bug Fixes
Chores