-
Notifications
You must be signed in to change notification settings - Fork 11
shard tests #657
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
shard tests #657
Changes from all commits
b2f1db7
663b23f
9ed9751
3d11c14
3252141
56664e6
542004f
2f207ed
9d89fdb
c35f1f2
9012b81
2ea844a
ab57df9
1bf951e
2524e46
e5b35c8
29346f3
4b4d829
5eee466
810345b
fdef447
3fa5194
fd25aac
66060fd
d7de7aa
52d9d7d
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 |
|---|---|---|
| @@ -1,32 +1,78 @@ | ||
| name: Test | ||
|
|
||
| on: | ||
| pull_request: | ||
| branches: | ||
| - main | ||
| push: | ||
| branches: | ||
| - main | ||
|
|
||
| concurrency: ${{ github.workflow }}-${{ github.ref }} | ||
| concurrency: | ||
| group: '${{ github.workflow }} @ ${{ github.event.pull_request.head.label || github.head_ref || github.ref }}' | ||
| cancel-in-progress: true | ||
|
|
||
| jobs: | ||
| test: | ||
| name: Test | ||
| timeout-minutes: 20 | ||
| determine-packages: | ||
| runs-on: ubuntu-latest | ||
| outputs: | ||
| packages: ${{ steps.set-packages.outputs.packages }} | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| - name: Get package names | ||
| id: set-packages | ||
| run: | | ||
| SEARCH_DIRS="plugins utils validate convert enrich import export" | ||
|
|
||
| PACKAGES=$( | ||
| for dir in $SEARCH_DIRS; do | ||
| if [ -d "$dir" ]; then | ||
| find "$dir" -maxdepth 2 -name "package.json" -exec sh -c ' | ||
| PACKAGE_NAME=$(jq -r .name {}) | ||
| if [ "$PACKAGE_NAME" != "null" ]; then | ||
| echo "$PACKAGE_NAME" | ||
| fi | ||
| ' \; | ||
| fi | ||
| done | jq -R -s -c 'split("\n")[:-1]' | ||
| ) | ||
|
|
||
| echo "packages=$PACKAGES" >> $GITHUB_OUTPUT | ||
| echo "Found packages: $PACKAGES" | ||
|
|
||
| test-packages: | ||
| needs: determine-packages | ||
| name: Test (${{ matrix.package }}) | ||
| runs-on: ubuntu-latest | ||
| if: ${{ github.head_ref != 'changeset-release/main' }} | ||
| strategy: | ||
| matrix: | ||
| package: ${{fromJson(needs.determine-packages.outputs.packages)}} | ||
| fail-fast: false | ||
| steps: | ||
| - name: Check out code | ||
| uses: actions/checkout@v3 | ||
| with: | ||
| fetch-depth: 2 | ||
| uses: actions/checkout@v4 | ||
|
|
||
| - name: Setup Node.js environment | ||
| uses: actions/setup-node@v3 | ||
| uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: 16 | ||
| node-version: 18 | ||
| cache: 'npm' | ||
|
|
||
| - name: Install dependencies | ||
| run: npm ci | ||
|
|
||
| - name: Build | ||
| run: npx turbo run build | ||
|
|
||
| - name: Test | ||
| run: npm run test | ||
| run: npx turbo run test --filter=${{ matrix.package }} | ||
|
|
||
| test: | ||
| needs: test-packages | ||
| name: Test | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: Check test results | ||
| run: | | ||
| echo "All package tests completed successfully" | ||
| exit 0 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| module.exports = { | ||
| testEnvironment: 'node', | ||
|
|
||
| transform: { | ||
| '^.+\\.tsx?$': 'ts-jest', | ||
| }, | ||
| setupFiles: ['../../test/dotenv-config.js'], | ||
| setupFilesAfterEnv: [ | ||
| '../../test/betterConsoleLog.js', | ||
| '../../test/unit.cleanup.js', | ||
| ], | ||
| testTimeout: 60_000, | ||
| globalSetup: '../../test/setup-global.js', | ||
| forceExit: true, | ||
| passWithNoTests: true, | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| module.exports = { | ||
| testEnvironment: 'node', | ||
|
|
||
| transform: { | ||
| '^.+\\.tsx?$': 'ts-jest', | ||
| }, | ||
| setupFiles: ['../../test/dotenv-config.js'], | ||
| setupFilesAfterEnv: [ | ||
| '../../test/betterConsoleLog.js', | ||
| '../../test/unit.cleanup.js', | ||
| ], | ||
| testTimeout: 60_000, | ||
| globalSetup: '../../test/setup-global.js', | ||
| forceExit: true, | ||
| passWithNoTests: true, | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| module.exports = { | ||
| testEnvironment: 'node', | ||
|
|
||
| transform: { | ||
| '^.+\\.tsx?$': 'ts-jest', | ||
| }, | ||
| setupFiles: ['../../test/dotenv-config.js'], | ||
| setupFilesAfterEnv: [ | ||
| '../../test/betterConsoleLog.js', | ||
| '../../test/unit.cleanup.js', | ||
| ], | ||
| testTimeout: 90_000, | ||
| globalSetup: '../../test/setup-global.js', | ||
| forceExit: true, | ||
| passWithNoTests: true, | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| module.exports = { | ||
| testEnvironment: 'node', | ||
|
|
||
| transform: { | ||
| '^.+\\.tsx?$': 'ts-jest', | ||
| }, | ||
| setupFiles: ['../../test/dotenv-config.js'], | ||
| setupFilesAfterEnv: [ | ||
| '../../test/betterConsoleLog.js', | ||
| '../../test/unit.cleanup.js', | ||
| ], | ||
| testTimeout: 60_000, | ||
|
Contributor
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. 💡 Codebase verification Reduce the The current
🔗 Analysis chainConsider reviewing the need for a 60-second test timeout. The To help determine if this timeout is necessary, we can check for long-running tests: This script will help identify any tests that take longer than 10 seconds to run, which might justify the 60-second timeout. 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Search for test files and analyze their execution time
fd -e test.ts -e spec.ts -e test.js -e spec.js | xargs grep -H "test(" | while read -r line; do
file=$(echo "$line" | cut -d':' -f1)
test_name=$(echo "$line" | sed -E 's/.*test\((.*)\).*/\1/')
echo "Running test: $test_name in $file"
start_time=$(date +%s%N)
npx jest "$file" -t "$test_name" > /dev/null 2>&1
end_time=$(date +%s%N)
duration=$(( (end_time - start_time) / 1000000 ))
if [ $duration -gt 10000 ]; then
echo "Long-running test detected: $test_name in $file (${duration}ms)"
fi
done
Length of output: 23582 |
||
| globalSetup: '../../test/setup-global.js', | ||
| forceExit: true, | ||
| passWithNoTests: true, | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -42,7 +42,9 @@ describe('externalConstraint()', () => { | |
|
|
||
| it('correctly assigns an error', async () => { | ||
| await createRecords(sheetId, defaultSimpleValueData) | ||
| await listener.waitFor('commit:created') | ||
| await listener.waitFor('commit:created', 2) | ||
| // Sleep for 1000ms to allow time for the constraint to be applied | ||
|
Contributor
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. Consider addressing the potential timing issue without using sleep. The comment suggests adding a sleep to allow time for the constraint to be applied. This indicates a potential race condition or timing issue in the test or implementation. Instead of using an arbitrary sleep, consider these alternatives:
These approaches would make the test more reliable and faster than using a sleep. |
||
| // await new Promise(resolve => setTimeout(resolve, 1000)); | ||
| const records = await getRecords(sheetId) | ||
| expect(records[0].values['name'].messages[0]).toMatchObject({ | ||
| type: 'error', | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| module.exports = { | ||
| testEnvironment: 'node', | ||
|
|
||
| transform: { | ||
| '^.+\\.tsx?$': 'ts-jest', | ||
| }, | ||
| setupFiles: ['../../test/dotenv-config.js'], | ||
| setupFilesAfterEnv: [ | ||
| '../../test/betterConsoleLog.js', | ||
| '../../test/unit.cleanup.js', | ||
| ], | ||
| testTimeout: 60_000, | ||
| globalSetup: '../../test/setup-global.js', | ||
| forceExit: true, | ||
| passWithNoTests: true, | ||
|
Comment on lines
+14
to
+15
Contributor
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. 🛠️ Refactor suggestion Consider the implications of forceExit and passWithNoTests. While these options can be useful in certain scenarios, they might mask underlying issues:
Consider the following alternatives:
If these options are necessary for specific reasons, please add comments explaining why they are needed. |
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| module.exports = { | ||
| testEnvironment: 'node', | ||
|
|
||
| transform: { | ||
| '^.+\\.tsx?$': 'ts-jest', | ||
| }, | ||
| setupFiles: ['../../test/dotenv-config.js'], | ||
| setupFilesAfterEnv: [ | ||
| '../../test/betterConsoleLog.js', | ||
| '../../test/unit.cleanup.js', | ||
| ], | ||
| testTimeout: 60_000, | ||
| globalSetup: '../../test/setup-global.js', | ||
| forceExit: true, | ||
| passWithNoTests: true, | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| module.exports = { | ||
| testEnvironment: 'node', | ||
|
|
||
| transform: { | ||
| '^.+\\.tsx?$': 'ts-jest', | ||
| }, | ||
| setupFiles: ['../../test/dotenv-config.js'], | ||
| setupFilesAfterEnv: [ | ||
| '../../test/betterConsoleLog.js', | ||
| '../../test/unit.cleanup.js', | ||
| ], | ||
| testTimeout: 60_000, | ||
| globalSetup: '../../test/setup-global.js', | ||
| forceExit: true, | ||
| passWithNoTests: true, | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| module.exports = { | ||
| testEnvironment: 'node', | ||
|
|
||
| transform: { | ||
| '^.+\\.tsx?$': 'ts-jest', | ||
| }, | ||
| setupFiles: ['../../test/dotenv-config.js'], | ||
| setupFilesAfterEnv: [ | ||
| '../../test/betterConsoleLog.js', | ||
| '../../test/unit.cleanup.js', | ||
| ], | ||
| testTimeout: 60_000, | ||
| globalSetup: '../../test/setup-global.js', | ||
| forceExit: true, | ||
| passWithNoTests: true, | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| module.exports = { | ||
| testEnvironment: 'node', | ||
|
|
||
| transform: { | ||
| '^.+\\.tsx?$': 'ts-jest', | ||
| }, | ||
| setupFiles: ['../../test/dotenv-config.js'], | ||
| setupFilesAfterEnv: [ | ||
| '../../test/betterConsoleLog.js', | ||
| '../../test/unit.cleanup.js', | ||
| ], | ||
| testTimeout: 60_000, | ||
| globalSetup: '../../test/setup-global.js', | ||
| forceExit: true, | ||
| passWithNoTests: true, | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| module.exports = { | ||
| testEnvironment: 'node', | ||
|
|
||
| transform: { | ||
| '^.+\\.tsx?$': 'ts-jest', | ||
| }, | ||
| setupFiles: ['../../test/dotenv-config.js'], | ||
| setupFilesAfterEnv: [ | ||
| '../../test/betterConsoleLog.js', | ||
| '../../test/unit.cleanup.js', | ||
| ], | ||
| testTimeout: 60_000, | ||
| globalSetup: '../../test/setup-global.js', | ||
| forceExit: true, | ||
| passWithNoTests: true, | ||
| } |
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.
💡 Codebase verification
Issues Detected:
npxcommand is not found, preventing Jest from running. Ensure that Jest is installed and thatnpxis available in the environment.*.test.tsor*.test.js) are present in the repository. This makes thepassWithNoTests: truesetting potentially problematic, as it allows the test suite to pass without executing any tests.🔗 Analysis chain
Review the necessity of forceExit and passWithNoTests options.
While these options can be useful in certain scenarios, they may mask underlying issues:
forceExit: trueforces Jest to exit after all tests complete. This can hide problems with asynchronous operations or resources not being properly closed.passWithNoTests: trueallows the test suite to pass when no tests are found. This might lead to false confidence if tests are accidentally skipped or not written.Let's investigate if these options are necessary:
Consider removing these options if possible, and address any underlying issues they might be masking. If they are necessary, add comments explaining why they are needed to prevent future confusion.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
Length of output: 159