Reverting workflow changes #2497
Conversation
…prayansh-talawa-admin into migration_to_vitest
…prayansh-talawa-admin into migration_to_vitest
…prayansh-talawa-admin into migration_to_vitest
…prayansh-talawa-admin into migration_to_vitest
…prayansh-talawa-admin into migration_to_vitest
…prayansh-talawa-admin into migration_to_vitest
WalkthroughThis pull request includes modifications to several configuration files and testing setups. The Changes
Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Our Pull Request Approval ProcessThanks for contributing! Testing Your CodeRemember, your PRs won't be reviewed until these criteria are met:
Our policies make our code better. ReviewersDo not assign reviewers. Our Queue Monitors will review your PR and assign them.
Reviewing Your CodeYour reviewer(s) will have the following roles:
CONTRIBUTING.mdRead our CONTRIBUTING.md file. Most importantly:
Other
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (13)
tsconfig.json (1)
21-21: Consider optimizing the include patterns.The current include patterns have some redundancy:
"src/App.tsx"is already covered by"src"- The location of
"setup.ts"is ambiguousConsider simplifying to:
- "include": ["src", "src/App.tsx", "setup.ts"], + "include": ["src", "setup.ts"],vitest.config.ts (3)
15-15: Consider expanding test file patterns.The current pattern only includes
.specfiles. Consider adding support for.testfiles as well, which is a common naming convention.- include: ['src/**/*.spec.{js,jsx,ts,tsx}'], + include: ['src/**/*.{spec,test}.{js,jsx,ts,tsx}'],
18-33: Enhance coverage configuration.While the current coverage configuration is good, consider these improvements:
- Add coverage thresholds to maintain code quality
- Include branch coverage reporting
- Consider excluding test setup files explicitly
coverage: { enabled: true, provider: 'istanbul', reportsDirectory: './coverage/vitest', + all: true, + branches: 80, + functions: 80, + lines: 80, + statements: 80, exclude: [ 'node_modules', 'dist', '**/*.{spec,test}.{js,jsx,ts,tsx}', 'coverage/**', '**/index.{js,ts}', '**/*.d.ts', 'src/test/**', 'vitest.config.ts', + 'src/setupTests.{ts,js}', ], reporter: ['text', 'html', 'text-summary', 'lcov'], },
14-34: Consider adding additional test configuration options.The current configuration misses some useful Vitest options that could improve the testing experience:
test: { include: ['src/**/*.spec.{js,jsx,ts,tsx}'], globals: true, environment: 'jsdom', + setupFiles: ['src/setupTests.ts'], + testTimeout: 10000, + maxConcurrency: 5, + passWithNoTests: false, coverage: { // ... existing coverage config }, },src/components/AddOn/AddOn.spec.tsx (3)
1-6: Consider enhancing the documentation headerWhile the documentation provides a good overview, it could be more specific about what aspects of the AddOn component are being tested and what testing strategies are employed.
Consider expanding it like this:
/** * Unit tests for the AddOn component. * * This file contains tests for the AddOn component to ensure it behaves as expected * under various scenarios. + * + * Testing Strategy: + * - Component rendering with default props + * - Component rendering with custom props + * - Integration with Redux store + * - Internationalization (i18n) functionality */
13-20: Consider reorganizing imports and setupThe imports are well-structured, but consider:
- Grouping related imports together (testing utilities, React core, third-party libraries)
- Moving the
StaticMockLinkinitialization closer to the test suite where it's usedConsider reorganizing like this:
// React and core dependencies import React from 'react'; import { BrowserRouter } from 'react-router-dom'; import { Provider } from 'react-redux'; + // Testing utilities import { render } from '@testing-library/react'; import '@testing-library/jest-dom'; import { describe, test, expect, vi } from 'vitest'; import { MockedProvider } from '@apollo/react-testing'; + // Application imports import { store } from 'state/store'; import AddOn from './AddOn'; import i18nForTest from 'utils/i18nForTest'; import { StaticMockLink } from 'utils/StaticMockLink'; - -const link = new StaticMockLink([], true);
Line range hint
39-70: Expand test coverage for better component validationWhile the current tests cover basic rendering scenarios, consider adding:
- Error handling tests
- Edge cases (empty/null props)
- User interaction tests
- Snapshot tests
Here's a suggested addition to your test suite:
test('should handle empty children prop gracefully', () => { const { getByTestId } = render( <MockedProvider addTypename={false} link={link}> <BrowserRouter> <Provider store={store}> <I18nextProvider i18n={i18nForTest}> <AddOn children={''} /> </I18nextProvider> </Provider> </BrowserRouter> </MockedProvider> ); const container = getByTestId('pluginContainer'); expect(container).toBeInTheDocument(); expect(container).not.toHaveTextContent(); // or expected fallback text }); test('should match snapshot', () => { const { container } = render( <MockedProvider addTypename={false} link={link}> <BrowserRouter> <Provider store={store}> <I18nextProvider i18n={i18nForTest}> <AddOn {...props} /> </I18nextProvider> </Provider> </BrowserRouter> </MockedProvider> ); expect(container).toMatchSnapshot(); });jest.config.js (2)
3-13: Optimize coverage collection patternsThe coverage collection patterns can be simplified:
!node_modulesand!distare redundant since they're outsidesrc/- The closing bracket formatting needs adjustment
Consider this optimization:
collectCoverageFrom: [ 'src/**/*.{ts,tsx}', '!src/index.tsx', - '!node_modules', - '!dist', '!**/*.{spec,test}.{js,jsx,ts,tsx}', '!coverage/**', '!**/index.{js,ts}', '!**/*.d.ts', '!src/test/**', - '!vitest.config.ts',], + '!vitest.config.ts' + ],🧰 Tools
🪛 eslint
[error] 13-13: Insert
⏎··(prettier/prettier)
Line range hint
1-82: Review architectural directionThe configuration changes suggest a transition to a dual-testing setup with Jest and Vitest, which is a significant architectural change. This appears to conflict with the PR's stated objective of "Reverting workflow changes". Please clarify:
- Is this intentionally part of a larger testing infrastructure change?
- If not, should these changes be split into a separate PR?
🧰 Tools
🪛 eslint
[error] 13-13: Insert
⏎··(prettier/prettier)
.github/workflows/pull-request.yml (4)
223-230: Enhance coverage merge error handlingThe current error handling for coverage merge could be improved to provide more context.
Consider this enhancement:
- name: Merge Coverage Reports if: steps.changed-files.outputs.only_changed != 'true' run: | mkdir -p coverage - if ! npx lcov-result-merger 'coverage/*/lcov.info' > 'coverage/lcov.info'; then - echo "Failed to merge coverage reports" - exit 1 - fi + if ! npx lcov-result-merger "coverage/${{ github.run_id }}/*/lcov.info" > 'coverage/lcov.info' 2>merge_error.log; then + echo "Failed to merge coverage reports. Error log:" + cat merge_error.log + exit 1 + fi🧰 Tools
🪛 yamllint (1.35.1)
[error] 230-230: trailing spaces
(trailing-spaces)
285-287: Refactor app health check logicThe health check logic is duplicated across Docker and non-Docker jobs. Consider creating a reusable composite action.
- Create
.github/actions/health-check/action.yml:name: 'Health Check' description: 'Check if application is running on specified port' inputs: port: description: 'Port to check' required: true timeout: description: 'Timeout in seconds' default: '120' runs: using: 'composite' steps: - run: | timeout=${{ inputs.timeout }} echo "Starting health check with ${timeout}s timeout" while ! nc -z localhost ${{ inputs.port }} && [ $timeout -gt 0 ]; do sleep 1 timeout=$((timeout-1)) if [ $((timeout % 10)) -eq 0 ]; then echo "Still waiting for app to start... ${timeout}s remaining" fi done if [ $timeout -eq 0 ]; then echo "Timeout waiting for application to start" exit 1 fi echo "App started successfully on port ${{ inputs.port }}" shell: bash
- Update the workflow to use this action:
- name: Check if Production App is running - run: | - timeout=120 - echo "Starting production health check with ${timeout}s timeout" - while ! nc -z localhost 4173 && [ $timeout -gt 0 ]; do - sleep 1 - timeout=$((timeout-1)) - if [ $((timeout % 10)) -eq 0 ]; then - echo "Still waiting for production app to start... ${timeout}s remaining" - fi - done - if [ $timeout -eq 0 ]; then - echo "Timeout waiting for production application to start" - exit 1 - fi - echo "Production app started successfully" + uses: ./.github/actions/health-check + with: + port: 4173Also applies to: 305-347
🧰 Tools
🪛 yamllint (1.35.1)
[warning] 285-285: wrong indentation: expected 4 but found 6
(indentation)
416-416: Add newline at end of fileThe file is missing a newline at the end.
Add a newline at the end of the file.
🧰 Tools
🪛 yamllint (1.35.1)
[error] 416-416: no new line character at the end of file
(new-line-at-end-of-file)
Target branch configuration needs immediate attention
The repository documentation explicitly states that PRs should be made against the
developbranch, but the workflow enforcesdevelop-postgresas the target. This inconsistency creates confusion and contradicts the documented branching strategy.
- Documentation in README/CONTRIBUTING guides users to use
developbranch- No mention of
develop-postgresin any documentation- Workflow enforces an undocumented branch requirement
🔗 Analysis chain
Line range hint
411-416: Review target branch configurationThe workflow enforces 'develop-postgres' as the target branch, which seems specific to a database migration phase.
Consider:
- Adding a comment explaining why 'develop-postgres' is currently the required target
- Creating a GitHub issue to track reverting this to 'develop' after migration
- Using repository variables/secrets to configure the target branch name
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if this is temporary during migration gh api repos/${{ github.repository }}/branches \ --jq '.[].name' | grep -E 'develop|develop-postgres'Length of output: 165
Script:
#!/bin/bash # Check repository configuration and branch setup fd -t f "^(package\.json|README\.md|CONTRIBUTING\.md|PR_GUIDELINES\.md)$" -X cat {} 2>/dev/null | grep -i "develop-postgres\|branch\|workflow" # Check other workflow files for branch configurations fd -t f "\.ya?ml$" -d 2 .github/workflows -X cat {} 2>/dev/null | grep -E "ref|branch.*develop"Length of output: 2948
🧰 Tools
🪛 yamllint (1.35.1)
[error] 416-416: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
.eslintignore(1 hunks).github/workflows/pull-request.yml(9 hunks)jest.config.js(3 hunks)package.json(5 hunks)src/components/AddOn/AddOn.spec.tsx(1 hunks)tsconfig.json(1 hunks)vitest.config.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .eslintignore
🧰 Additional context used
🪛 yamllint (1.35.1)
.github/workflows/pull-request.yml
[error] 49-49: trailing spaces
(trailing-spaces)
[error] 60-60: trailing spaces
(trailing-spaces)
[error] 63-63: trailing spaces
(trailing-spaces)
[error] 92-92: trailing spaces
(trailing-spaces)
[error] 133-133: trailing spaces
(trailing-spaces)
[error] 136-136: trailing spaces
(trailing-spaces)
[error] 157-157: trailing spaces
(trailing-spaces)
[error] 164-164: trailing spaces
(trailing-spaces)
[error] 204-204: trailing spaces
(trailing-spaces)
[error] 208-208: trailing spaces
(trailing-spaces)
[error] 230-230: trailing spaces
(trailing-spaces)
[error] 231-231: trailing spaces
(trailing-spaces)
[error] 248-248: duplication of key "fail_ci_if_error" in mapping
(key-duplicates)
[error] 271-271: trailing spaces
(trailing-spaces)
[error] 277-277: trailing spaces
(trailing-spaces)
[error] 279-279: trailing spaces
(trailing-spaces)
[warning] 285-285: wrong indentation: expected 4 but found 6
(indentation)
[error] 416-416: no new line character at the end of file
(new-line-at-end-of-file)
🪛 eslint
jest.config.js
[error] 13-13: Insert ⏎··
(prettier/prettier)
🔇 Additional comments (8)
tsconfig.json (1)
22-22: LGTM! Appropriate exclusion patterns added.
The exclude patterns are well-chosen:
node_modules: Prevents processing of external dependenciesdist: Excludes build outputvitest.config.ts: Correctly excludes the Vitest configuration file
vitest.config.ts (2)
1-4: LGTM! All necessary imports are present.
The imports are well-organized and include all required dependencies for React testing with Vitest.
7-13: Verify if additional Node.js polyfills are required.
The current configuration only includes the 'events' polyfill. Consider if your tests require other common Node.js built-ins.
Let's check if any tests use other Node.js built-ins:
✅ Verification successful
Current Node.js polyfills configuration is sufficient
The only Node.js built-in module usage found is 'fs', which is exclusively used in test setup and configuration files. These files are not part of the test execution environment as they are used for development setup. The current polyfill configuration including only 'events' is appropriate for the actual test suite requirements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for usage of common Node.js built-ins in test files
# Check for usage of common Node.js built-ins in test files
echo "Searching for Node.js built-in usage in tests..."
rg -t ts -t tsx -t js -t jsx "require\(['\"](?:path|util|stream|buffer|fs)" "src/**/*.{spec,test}.{ts,tsx,js,jsx}"
# Check for direct references
rg -t ts -t tsx -t js -t jsx "(?:path|util|stream|buffer|fs)\." "src/**/*.{spec,test}.{ts,tsx,js,jsx}"
Length of output: 386
Script:
#!/bin/bash
# Let's try a different approach using fd to find test files first
# Then use ripgrep without file type restrictions
# Find all test files
echo "Test files in the repository:"
fd -e ts -e tsx -e js -e jsx ".(test|spec)\."
# Search for Node.js built-ins in these files
echo -e "\nSearching for Node.js built-in usage..."
fd -e ts -e tsx -e js -e jsx ".(test|spec)\." -x rg -l "require\(['\"](?:path|util|stream|buffer|fs)" {} \;
fd -e ts -e tsx -e js -e jsx ".(test|spec)\." -x rg -l "(?:path|util|stream|buffer|fs)\." {} \;
# Also search for imports of these modules
echo -e "\nSearching for ES6 imports of Node.js built-ins..."
fd -e ts -e tsx -e js -e jsx ".(test|spec)\." -x rg -l "import.*from ['\"](?:path|util|stream|buffer|fs)['\"]" {} \;
Length of output: 11936
Script:
#!/bin/bash
# Let's examine the specific files that use Node.js built-ins to understand their usage
echo "Checking setupTalawaWebSocketUrl.test.ts:"
rg -A 5 "import.*from ['\"](?:path|util|stream|buffer|fs)['\"]" "src/setup/askForTalawaApiUrl/setupTalawaWebSocketUrl.test.ts"
echo -e "\nChecking checkEnvFile.test.ts:"
rg -A 5 "import.*from ['\"](?:path|util|stream|buffer|fs)['\"]" "src/setup/checkEnvFile/checkEnvFile.test.ts"
rg "(?:path|util|stream|buffer|fs)\." "src/setup/checkEnvFile/checkEnvFile.test.ts"
# Also check the implementation files to understand the context
echo -e "\nChecking related implementation files:"
rg -A 5 "import.*from ['\"](?:path|util|stream|buffer|fs)['\"]" "src/setup/checkEnvFile/checkEnvFile.ts" "src/setup/askForTalawaApiUrl/setupTalawaWebSocketUrl.ts"
Length of output: 1606
src/components/AddOn/AddOn.spec.tsx (1)
14-14: Verify Vitest configuration and test runner setup
Ensure that Vitest is properly configured in the project for running these tests.
jest.config.js (1)
43-44: Verify necessity of new module mappings
The addition of new module mappings for SVG and PDF generator mocks seems unrelated to the PR objective of reverting workflow changes. Please clarify if these mappings are essential.
Let's check for usage of these modules:
✅ Verification successful
Module mappings are necessary for test coverage
The verification confirms these mappings are essential:
- SVG imports are used across 15 files in the codebase, primarily in UI components and screens
@pdfme/generatoris used insrc/components/CheckIn/TableRow.tsx
These Jest module mappings are required to properly mock these dependencies during test execution, regardless of the PR's primary objective of workflow reversion. Without these mocks, tests involving these components would fail.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for SVG imports and PDF generator usage
echo "Files importing SVG:"
rg -l "import.*\.svg" src/
echo "Files using @pdfme/generator:"
rg -l "@pdfme/generator" src/
Length of output: 927
package.json (2)
158-158: Verify Vitest version compatibility
The specific version of Vitest (^2.1.5) should be verified for compatibility with:
- React testing requirements
- Coverage reporting tools
- Other testing utilities
Let's check for potential compatibility issues:
✅ Verification successful
The initial check shows Vitest's peer dependencies. Let's verify:
- If we have compatible Node.js version specified
- If we have any conflicting testing dependencies
- How Vitest is being used in the codebase
This will help us determine if there are any compatibility concerns with the current setup.
Vitest version 2.1.5 is compatible with the project setup
The verification shows that:
- Node.js compatibility is satisfied with
@types/node: ^22.9.0which meets Vitest's requirement of^18.0.0 || >=20.0.0 - The project has proper testing setup with complementary tools:
@vitest/coverage-istanbul: ^2.1.5(matching Vitest version)- React testing utilities (
@testing-library/react,@testing-library/jest-dom)
- Vitest is already being used in test files (e.g.,
src/components/AddOn/AddOn.spec.tsx)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for Vitest compatibility requirements
echo "Checking Vitest peer dependencies..."
npm info [email protected] peerDependencies
Length of output: 282
Script:
#!/bin/bash
# Check for related testing dependencies and Node.js version
echo "Package.json dependencies:"
cat package.json | jq '.dependencies, .devDependencies'
# Check for test files using Vitest
echo -e "\nChecking test files:"
rg -l "from 'vitest'"
rg -l "from \"vitest\""
Length of output: 3848
78-80: Clarify testing framework strategy
The addition of Vitest scripts alongside existing Jest configuration suggests a transition in testing frameworks. This dual setup might cause confusion. Consider:
- Documenting the migration strategy
- Ensuring consistent coverage reporting between both frameworks
- Adding a plan for eventual deprecation of Jest tests
Let's verify the testing setup:
✅ Verification successful
Let me check the test files to determine if they're using Jest or Vitest syntax.
Let me check for configuration files to understand the testing setup better.
Migration in progress - both Jest and Vitest configurations present
The codebase is currently in a transitional state with both testing frameworks:
- Jest is the primary framework (vast majority of 150+ test files use Jest)
- Only one test file (
AddOn.spec.tsx) currently uses Vitest - Both
jest.config.jsandvitest.config.tsconfigurations exist - Package.json shows both Jest and Vitest test scripts
This appears to be an intentional, gradual migration strategy rather than a confusing setup. The parallel configurations allow for:
- Testing new components with Vitest while maintaining existing Jest tests
- Incremental migration of existing tests without disrupting the test suite
- Validation of Vitest setup before full migration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for mixed test files
echo "Checking for mixed test configurations..."
rg -l "describe|test|it|expect" -g "**/*.test.*" -g "**/*.spec.*"
Length of output: 11231
Script:
#!/bin/bash
# Check for Jest specific imports/configurations
echo "Checking for Jest specific patterns..."
rg -l "@jest|jest\." -g "**/*.test.*" -g "**/*.spec.*" -g "**/jest.config.*" -g "**/vitest.config.*"
echo -e "\nChecking for Vitest specific patterns..."
rg -l "import.*vitest|from 'vitest'" -g "**/*.test.*" -g "**/*.spec.*" -g "**/vitest.config.*"
Length of output: 8590
Script:
#!/bin/bash
# Check for test configuration files
echo "Checking for test configuration files..."
fd -e js -e ts -e json "jest.config|vitest.config"
echo -e "\nChecking package.json scripts..."
rg "test.*jest|test.*vitest" package.json
Length of output: 565
.github/workflows/pull-request.yml (1)
49-52:
Review the condition for format check and fix
The condition steps.changed-files.outputs.only_changed != 'true' seems counterintuitive. The formatting check and fix should run on changed files, but the current condition suggests otherwise.
Consider updating the condition to:
- - name: Check formatting
- if: steps.changed-files.outputs.only_changed != 'true'
+ - name: Check formatting
+ if: steps.changed-files.outputs.any_changed == 'true'🧰 Tools
🪛 yamllint (1.35.1)
[error] 49-49: trailing spaces
(trailing-spaces)
| vi.mock('state/store', () => ({ | ||
| store: { | ||
| // Mock store configuration if needed | ||
| getState: vi.fn(), | ||
| subscribe: vi.fn(), | ||
| dispatch: vi.fn(), | ||
| }, | ||
| })); | ||
|
|
||
| vi.mock('utils/i18nForTest', () => ({ | ||
| __esModule: true, | ||
| default: vi.fn(() => ({ | ||
| t: (key: string) => key, | ||
| })), | ||
| })); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance mock implementations for better test coverage
The current mock implementations could be improved:
- The store mock lacks type definitions and could be expanded
- The i18n mock could be more type-safe
Consider enhancing the mocks:
vi.mock('state/store', () => ({
store: {
- // Mock store configuration if needed
+ // Implement a more complete store mock
getState: vi.fn(() => ({
+ // Add your initial state here
+ user: { /* mock user state */ },
+ plugins: { /* mock plugins state */ }
})),
subscribe: vi.fn(),
dispatch: vi.fn(),
},
}));
vi.mock('utils/i18nForTest', () => ({
__esModule: true,
- default: vi.fn(() => ({
- t: (key: string) => key,
- })),
+ default: vi.fn(() => {
+ type TranslationKey = string;
+ return {
+ t: (key: TranslationKey): string => key,
+ // Add other i18n methods as needed
+ };
+ }),
}));Committable suggestion skipped: line range outside the PR's diff.
| // setupFiles: ['react-app-polyfill/jsdom'], | ||
| setupFiles: ['whatwg-fetch'], | ||
| setupFilesAfterEnv: ['<rootDir>/src/setupTests.ts'], | ||
| testMatch: [ | ||
| '<rootDir>/src/**/__tests__/**/*.{js,jsx,ts,tsx}', | ||
| '<rootDir>/src/**/*.{spec,test}.{js,jsx,ts,tsx}', | ||
| '<rootDir>/src/**/*.test.{js,jsx,ts,tsx}', |
There was a problem hiding this comment.
💡 Codebase verification
Update test pattern to include both .spec and .test files
The verification confirms that there are existing .spec files in the codebase that would be excluded by the new pattern:
src/Constant/constant.spec.tssrc/components/AddOn/AddOn.spec.tsx
The current change would break these test files by preventing them from being discovered by Jest. The pattern should be reverted to include both .spec and .test extensions.
🔗 Analysis chain
Verify impact on existing test files
The change from *.{spec,test} to *.test pattern might break existing spec files. This seems inconsistent with the PR objective of reverting workflow changes.
Let's check for any spec files that might be affected:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for .spec.{js,jsx,ts,tsx} files that might be affected
echo "Searching for spec files that might be excluded by the new pattern:"
fd -e spec.js -e spec.jsx -e spec.ts -e spec.tsx . src/
Length of output: 268
| "vite-plugin-node-polyfills": "^0.22.0", | ||
| "vite-tsconfig-paths": "^5.1.3", |
There was a problem hiding this comment.
💡 Codebase verification
The vite-plugin-node-polyfills package is not configured in the Vite setup
The Vite configuration file shows that while vite-tsconfig-paths is properly configured as a plugin, the newly added vite-plugin-node-polyfills package is not included in the plugins array. This needs to be addressed to ensure the polyfills are active.
- Add
nodePolyfills()to the plugins array inconfig/vite.config.ts - Import the plugin:
import { nodePolyfills } from 'vite-plugin-node-polyfills'
🔗 Analysis chain
Review Vite configuration changes
The addition of vite-plugin-node-polyfills and update to vite-tsconfig-paths requires verification of the Vite configuration:
- Ensure polyfills are properly configured
- Verify path aliases are working correctly
Let's check the Vite configuration:
Let's examine the Vite configuration file to verify the polyfills and path aliases setup.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Look for Vite configuration
echo "Checking Vite configuration..."
fd -g "*vite.config.*"
Length of output: 117
Script:
#!/bin/bash
# Check the content of Vite configuration
cat config/vite.config.ts
Length of output: 799
| @@ -40,6 +40,7 @@ | |||
| "i18next-http-backend": "^2.6.1", | |||
| "inquirer": "^8.0.0", | |||
| "js-cookie": "^3.0.1", | |||
| "lcov-result-merger": "^5.0.1", | |||
There was a problem hiding this comment.
💡 Codebase verification
Based on the gathered information and the learning about merging coverage reports from Jest and Vitest, I can now provide the final response:
Coverage Reports Need Merge Configuration
The codebase has both Jest and Vitest configured to generate lcov reports in separate directories (./coverage/jest and ./coverage/vitest), but there's no script to merge these reports despite having lcov-result-merger installed. You need to:
- Add a merge script in package.json:
"merge-coverage": "lcov-result-merger './coverage/**/lcov.info' './coverage/lcov.info'" - Create a combined coverage script:
"coverage": "npm run test:vitest:coverage && npm run test && npm run merge-coverage"
This ensures proper consolidation of coverage reports from both testing frameworks.
🔗 Analysis chain
Verify coverage report configuration
The addition of lcov-result-merger and @vitest/coverage-istanbul suggests changes in coverage reporting. Ensure that:
- Coverage reports from both Jest and Vitest are properly merged
- Coverage thresholds are consistently applied
Let's check the coverage configuration:
Also applies to: 138-138
Let me check the content of these configuration files to verify the coverage setup.
Let me check for any scripts that handle the merging of coverage reports.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Look for coverage configuration
echo "Checking coverage configuration..."
rg -l "coverage|\.lcov|istanbul" -g "*.config.*"
Length of output: 161
Script:
#!/bin/bash
# Check coverage configuration in both files
echo "=== Jest Coverage Config ==="
cat jest.config.js
echo -e "\n=== Vitest Coverage Config ==="
cat vitest.config.ts
# Check for coverage merge scripts
echo -e "\n=== Coverage Scripts in package.json ==="
rg "coverage" package.json
Length of output: 3797
Script:
#!/bin/bash
# Check for coverage merge scripts in package.json
echo "=== Package.json Scripts ==="
cat package.json | jq '.scripts'
# Check if there are any existing coverage merge commands in the codebase
echo -e "\n=== Searching for coverage merge commands ==="
rg -i "merge.*coverage|coverage.*merge" -g '!package-lock.json'
Length of output: 1418
| verbose: true | ||
| fail_ci_if_error: false | ||
| files: './coverage/lcov.info' | ||
| name: '${{env.CODECOV_UNIQUE_NAME}}' | ||
| fail_ci_if_error: false |
There was a problem hiding this comment.
Fix duplicate configuration in Codecov upload
There's a duplicate fail_ci_if_error key in the Codecov configuration.
Remove the duplicate:
- name: Present and Upload coverage to Codecov as ${{env.CODECOV_UNIQUE_NAME}}
uses: codecov/codecov-action@v4
with:
token: ${{ secrets.CODECOV_TOKEN }}
verbose: true
- fail_ci_if_error: false
files: './coverage/lcov.info'
name: '${{env.CODECOV_UNIQUE_NAME}}'
- fail_ci_if_error: falseCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 yamllint (1.35.1)
[error] 248-248: duplication of key "fail_ci_if_error" in mapping
(key-duplicates)
| - name: Run Jest Tests | ||
| if: steps.changed-files.outputs.only_changed != 'true' | ||
| env: | ||
| NODE_V8_COVERAGE: './coverage/jest' | ||
| run: | | ||
| npm run test -- --watchAll=false --coverage | ||
|
|
||
| - name: Run tests | ||
| - name: Run Vitest Tests | ||
| if: steps.changed-files.outputs.only_changed != 'true' | ||
| run: npm run test -- --watchAll=false --coverage | ||
| env: | ||
| NODE_V8_COVERAGE: './coverage/vitest' | ||
| run: | | ||
| npm run test:vitest:coverage |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve test execution configuration
The current setup has several potential issues:
- Both Jest and Vitest tests use
steps.changed-files.outputs.only_changed != 'true'which might skip tests unnecessarily - Coverage directories are hardcoded which could cause conflicts
Consider these improvements:
- name: Run Jest Tests
- if: steps.changed-files.outputs.only_changed != 'true'
+ if: steps.changed-files.outputs.any_changed == 'true'
env:
- NODE_V8_COVERAGE: './coverage/jest'
+ NODE_V8_COVERAGE: './coverage/${{ github.run_id }}/jest'
run: |
npm run test -- --watchAll=false --coverage
- name: Run Vitest Tests
- if: steps.changed-files.outputs.only_changed != 'true'
+ if: steps.changed-files.outputs.any_changed == 'true'
env:
- NODE_V8_COVERAGE: './coverage/vitest'
+ NODE_V8_COVERAGE: './coverage/${{ github.run_id }}/vitest'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Run Jest Tests | |
| if: steps.changed-files.outputs.only_changed != 'true' | |
| env: | |
| NODE_V8_COVERAGE: './coverage/jest' | |
| run: | | |
| npm run test -- --watchAll=false --coverage | |
| - name: Run tests | |
| - name: Run Vitest Tests | |
| if: steps.changed-files.outputs.only_changed != 'true' | |
| run: npm run test -- --watchAll=false --coverage | |
| env: | |
| NODE_V8_COVERAGE: './coverage/vitest' | |
| run: | | |
| npm run test:vitest:coverage | |
| - name: Run Jest Tests | |
| if: steps.changed-files.outputs.any_changed == 'true' | |
| env: | |
| NODE_V8_COVERAGE: './coverage/${{ github.run_id }}/jest' | |
| run: | | |
| npm run test -- --watchAll=false --coverage | |
| - name: Run Vitest Tests | |
| if: steps.changed-files.outputs.any_changed == 'true' | |
| env: | |
| NODE_V8_COVERAGE: './coverage/${{ github.run_id }}/vitest' | |
| run: | | |
| npm run test:vitest:coverage |
What kind of change does this PR introduce?
Issue Number:
Fixes #
Did you add tests for your changes?
No
Snapshots/Videos:
If relevant, did you update the documentation?
Summary
Does this PR introduce a breaking change?
Other information
Have you read the contributing guide?
Summary by CodeRabbit
New Features
Improvements
Documentation
Chores