From c4018e9e1d76cae664aaa836dd49fdc44e808550 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 17 Apr 2024 14:08:23 +0200 Subject: [PATCH 1/7] test(ci): Adjust `detectFlakyTests` to account for multiple tests in a file --- .../scripts/detectFlakyTests.ts | 81 +++++++++++++++---- 1 file changed, 64 insertions(+), 17 deletions(-) diff --git a/dev-packages/browser-integration-tests/scripts/detectFlakyTests.ts b/dev-packages/browser-integration-tests/scripts/detectFlakyTests.ts index 299ebb002692..3b94e7f26838 100644 --- a/dev-packages/browser-integration-tests/scripts/detectFlakyTests.ts +++ b/dev-packages/browser-integration-tests/scripts/detectFlakyTests.ts @@ -1,7 +1,28 @@ import * as childProcess from 'child_process'; +import * as fs from 'fs'; import * as path from 'path'; import * as glob from 'glob'; +/** + * The number of browsers we run the tests in. + */ +const NUM_BROWSERS = 4; + +/** + * Assume that each test runs for 3s. + */ +const ASSUMED_TEST_DURATION_SECONDS = 3; + +/** + * We keep the runtime of the detector if possible under 30min. + */ +const MAX_TARGET_TEST_RUNTIME_SECONDS = 30 * 60; + +/** + * Running one test 50x is what we consider enough to detect flakiness. + */ +const MAX_PER_TEST_RUN_COUNT = 50; + async function run(): Promise { let testPaths: string[] = []; @@ -20,23 +41,8 @@ ${changedPaths.join('\n')} } } - let runCount: number; - if (process.env.TEST_RUN_COUNT === 'AUTO') { - // No test paths detected: run everything 5x - runCount = 5; - - if (testPaths.length > 0) { - // Run everything up to 100x, assuming that total runtime is less than 60min. - // We assume an average runtime of 3s per test, times 4 (for different browsers) = 12s per detected testPaths - // We want to keep overall runtime under 30min - const testCount = testPaths.length * 4; - const expectedRuntimePerTestPath = testCount * 3; - const expectedRuntime = Math.floor((30 * 60) / expectedRuntimePerTestPath); - runCount = Math.min(50, Math.max(expectedRuntime, 5)); - } - } else { - runCount = parseInt(process.env.TEST_RUN_COUNT || '10'); - } + const runCount = getPerTestRunCount(testPaths); + console.log(`Running tests ${runCount} times each.`); const cwd = path.join(__dirname, '../'); @@ -88,6 +94,29 @@ ${changedPaths.join('\n')} console.log(`☑️ All tests passed.`); } +/** + * Returns how many time one test should run based on the chosen mode and a bunch of heuristics + */ +function getPerTestRunCount(testPaths: string[]) { + if (process.env.TEST_RUN_COUNT === 'AUTO' && testPaths.length > 0) { + // Run everything up to 100x, assuming that total runtime is less than 60min. + // We assume an average runtime of 3s per test, times 4 (for different browsers) = 12s per detected testPaths + // We want to keep overall runtime under 30min + const estimatedNumberOfTests = testPaths.map(getApproximateNumberOfTests).reduce((a, b) => a + b); + console.log(`Estimated number of tests: ${estimatedNumberOfTests}`); + + // tests are usually run against all browsers we test with, so let's assume this + const testRunCount = estimatedNumberOfTests * NUM_BROWSERS; + + const estimatedTestRuntime = testRunCount * ASSUMED_TEST_DURATION_SECONDS; + const expectedRuntime = Math.floor(MAX_TARGET_TEST_RUNTIME_SECONDS / estimatedTestRuntime); + + return Math.min(MAX_PER_TEST_RUN_COUNT, Math.max(expectedRuntime, 5)); + } + + return parseInt(process.env.TEST_RUN_COUNT || '5'); +} + function getTestPaths(): string[] { const paths = glob.sync('suites/**/test.{ts,js}', { cwd: path.join(__dirname, '../'), @@ -111,4 +140,22 @@ function logError(error: unknown) { } } +/** + * Definitely not bulletproof way of getting the number of tests in a file :D + * We simply match on `it(`, `test(`, etc and count the matches. + * + * Note: This test completely disregards parameterized tests (`it.each`, etc) or + * skipped/disabled tests and other edge cases. It's just a rough estimate. + */ +function getApproximateNumberOfTests(testPath: string): number { + try { + const content = fs.readFileSync(testPath, 'utf-8'); + const matches = content.match(/it\(|test\(|sentryTest\(/g); + return Math.max(matches ? matches.length : 1, 1); + } catch (e) { + console.error(`Could not read file ${testPath}`); + return 1; + } +} + run(); From 84279f7fd4be83b55f53056ce4b12ac53acd9374 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 17 Apr 2024 14:10:09 +0200 Subject: [PATCH 2/7] add a couple of changed tests --- .../suites/tracing/stringSampleRate/test.ts | 2 ++ .../suites/tracing/trace-lifetime/navigation/test.ts | 1 + 2 files changed, 3 insertions(+) diff --git a/dev-packages/browser-integration-tests/suites/tracing/stringSampleRate/test.ts b/dev-packages/browser-integration-tests/suites/tracing/stringSampleRate/test.ts index 1b411d16e85b..548edb42a22c 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/stringSampleRate/test.ts +++ b/dev-packages/browser-integration-tests/suites/tracing/stringSampleRate/test.ts @@ -4,6 +4,8 @@ import { sentryTest } from '../../../utils/fixtures'; import { envelopeRequestParser, shouldSkipTracingTest, waitForTransactionRequestOnUrl } from '../../../utils/helpers'; sentryTest('parses a string sample rate', async ({ getLocalTestUrl, page }) => { + // xxx test comment, remove before merging + if (shouldSkipTracingTest()) { sentryTest.skip(); } diff --git a/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/navigation/test.ts b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/navigation/test.ts index f24ca0507c66..c61c2d2fad00 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/navigation/test.ts +++ b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/navigation/test.ts @@ -8,6 +8,7 @@ import { } from '../../../../utils/helpers'; sentryTest('creates a new trace on each navigation', async ({ getLocalTestPath, page }) => { + // xxx test comment, remove before merging if (shouldSkipTracingTest()) { sentryTest.skip(); } From 21ab1779548d5bb32db7cc64bf86d56730d470e4 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 17 Apr 2024 14:24:54 +0200 Subject: [PATCH 3/7] fix path? --- .../scripts/detectFlakyTests.ts | 20 ++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/dev-packages/browser-integration-tests/scripts/detectFlakyTests.ts b/dev-packages/browser-integration-tests/scripts/detectFlakyTests.ts index 3b94e7f26838..7010c6674011 100644 --- a/dev-packages/browser-integration-tests/scripts/detectFlakyTests.ts +++ b/dev-packages/browser-integration-tests/scripts/detectFlakyTests.ts @@ -20,10 +20,13 @@ const MAX_TARGET_TEST_RUNTIME_SECONDS = 30 * 60; /** * Running one test 50x is what we consider enough to detect flakiness. + * Running one test 5x is the bare minimum */ const MAX_PER_TEST_RUN_COUNT = 50; +const MIN_PER_TEST_RUN_COUNT = 5; async function run(): Promise { + console.log('xxx cwd', process.cwd()); let testPaths: string[] = []; const changedPaths: string[] = process.env.CHANGED_TEST_PATHS ? JSON.parse(process.env.CHANGED_TEST_PATHS) : []; @@ -41,8 +44,8 @@ ${changedPaths.join('\n')} } } - const runCount = getPerTestRunCount(testPaths); - console.log(`Running tests ${runCount} times each.`); + const repeatEachCount = getPerTestRunCount(testPaths); + console.log(`Running tests ${repeatEachCount} times each.`); const cwd = path.join(__dirname, '../'); @@ -51,7 +54,7 @@ ${changedPaths.join('\n')} const cp = childProcess.spawn( `npx playwright test ${ testPaths.length ? testPaths.join(' ') : './suites' - } --reporter='line' --repeat-each ${runCount}`, + } --reporter='line' --repeat-each ${repeatEachCount}`, { shell: true, cwd }, ); @@ -107,11 +110,15 @@ function getPerTestRunCount(testPaths: string[]) { // tests are usually run against all browsers we test with, so let's assume this const testRunCount = estimatedNumberOfTests * NUM_BROWSERS; + console.log('Estimated test run count:', testRunCount); const estimatedTestRuntime = testRunCount * ASSUMED_TEST_DURATION_SECONDS; + console.log('Estimated test runtime:', estimatedTestRuntime); + const expectedRuntime = Math.floor(MAX_TARGET_TEST_RUNTIME_SECONDS / estimatedTestRuntime); + console.log('Expected test run count:', expectedRuntime); - return Math.min(MAX_PER_TEST_RUN_COUNT, Math.max(expectedRuntime, 5)); + return Math.min(MAX_PER_TEST_RUN_COUNT, Math.max(expectedRuntime, MIN_PER_TEST_RUN_COUNT)); } return parseInt(process.env.TEST_RUN_COUNT || '5'); @@ -149,7 +156,10 @@ function logError(error: unknown) { */ function getApproximateNumberOfTests(testPath: string): number { try { - const content = fs.readFileSync(testPath, 'utf-8'); + const content = fs.readFileSync( + path.join(process.cwd(), 'dev-packages', 'browser-integration-tests', testPath), + 'utf-8', + ); const matches = content.match(/it\(|test\(|sentryTest\(/g); return Math.max(matches ? matches.length : 1, 1); } catch (e) { From 4fc8e4734e585206bde3f8325f1d7c957585feea Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 17 Apr 2024 14:33:48 +0200 Subject: [PATCH 4/7] maybe now fix paths? --- .../browser-integration-tests/scripts/detectFlakyTests.ts | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/dev-packages/browser-integration-tests/scripts/detectFlakyTests.ts b/dev-packages/browser-integration-tests/scripts/detectFlakyTests.ts index 7010c6674011..961e90db64cd 100644 --- a/dev-packages/browser-integration-tests/scripts/detectFlakyTests.ts +++ b/dev-packages/browser-integration-tests/scripts/detectFlakyTests.ts @@ -26,7 +26,6 @@ const MAX_PER_TEST_RUN_COUNT = 50; const MIN_PER_TEST_RUN_COUNT = 5; async function run(): Promise { - console.log('xxx cwd', process.cwd()); let testPaths: string[] = []; const changedPaths: string[] = process.env.CHANGED_TEST_PATHS ? JSON.parse(process.env.CHANGED_TEST_PATHS) : []; @@ -156,10 +155,7 @@ function logError(error: unknown) { */ function getApproximateNumberOfTests(testPath: string): number { try { - const content = fs.readFileSync( - path.join(process.cwd(), 'dev-packages', 'browser-integration-tests', testPath), - 'utf-8', - ); + const content = fs.readFileSync(path.join(process.cwd(), testPath, 'test.ts'), 'utf-8'); const matches = content.match(/it\(|test\(|sentryTest\(/g); return Math.max(matches ? matches.length : 1, 1); } catch (e) { From 796b3bf46790a83efc062a6e29b641d4d5dadd97 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 17 Apr 2024 14:54:58 +0200 Subject: [PATCH 5/7] reduce to 3 browsers --- .../browser-integration-tests/scripts/detectFlakyTests.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dev-packages/browser-integration-tests/scripts/detectFlakyTests.ts b/dev-packages/browser-integration-tests/scripts/detectFlakyTests.ts index 961e90db64cd..d75b2be0af59 100644 --- a/dev-packages/browser-integration-tests/scripts/detectFlakyTests.ts +++ b/dev-packages/browser-integration-tests/scripts/detectFlakyTests.ts @@ -6,7 +6,7 @@ import * as glob from 'glob'; /** * The number of browsers we run the tests in. */ -const NUM_BROWSERS = 4; +const NUM_BROWSERS = 3; /** * Assume that each test runs for 3s. From 674f026e2f8d5ac65e76d51e947886f4f8583f13 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 17 Apr 2024 15:10:30 +0200 Subject: [PATCH 6/7] reword logs --- .../scripts/detectFlakyTests.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/dev-packages/browser-integration-tests/scripts/detectFlakyTests.ts b/dev-packages/browser-integration-tests/scripts/detectFlakyTests.ts index d75b2be0af59..8fc5d9ff2e92 100644 --- a/dev-packages/browser-integration-tests/scripts/detectFlakyTests.ts +++ b/dev-packages/browser-integration-tests/scripts/detectFlakyTests.ts @@ -109,15 +109,15 @@ function getPerTestRunCount(testPaths: string[]) { // tests are usually run against all browsers we test with, so let's assume this const testRunCount = estimatedNumberOfTests * NUM_BROWSERS; - console.log('Estimated test run count:', testRunCount); + console.log(`Estimated test runs for one round: ${testRunCount}`); const estimatedTestRuntime = testRunCount * ASSUMED_TEST_DURATION_SECONDS; - console.log('Estimated test runtime:', estimatedTestRuntime); + console.log(`Estimated test runtime: ${estimatedTestRuntime}s`); - const expectedRuntime = Math.floor(MAX_TARGET_TEST_RUNTIME_SECONDS / estimatedTestRuntime); - console.log('Expected test run count:', expectedRuntime); + const expectedPerTestRunCount = Math.floor(MAX_TARGET_TEST_RUNTIME_SECONDS / estimatedTestRuntime); + console.log(`Expected per-test run count: ${expectedPerTestRunCount}`); - return Math.min(MAX_PER_TEST_RUN_COUNT, Math.max(expectedRuntime, MIN_PER_TEST_RUN_COUNT)); + return Math.min(MAX_PER_TEST_RUN_COUNT, Math.max(expectedPerTestRunCount, MIN_PER_TEST_RUN_COUNT)); } return parseInt(process.env.TEST_RUN_COUNT || '5'); From 5a879dcc738ba5bcf5c65c3bf6631c76b4f12c78 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 17 Apr 2024 15:25:11 +0200 Subject: [PATCH 7/7] aaand cleanup --- .../suites/tracing/stringSampleRate/test.ts | 2 -- .../suites/tracing/trace-lifetime/navigation/test.ts | 1 - 2 files changed, 3 deletions(-) diff --git a/dev-packages/browser-integration-tests/suites/tracing/stringSampleRate/test.ts b/dev-packages/browser-integration-tests/suites/tracing/stringSampleRate/test.ts index 548edb42a22c..1b411d16e85b 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/stringSampleRate/test.ts +++ b/dev-packages/browser-integration-tests/suites/tracing/stringSampleRate/test.ts @@ -4,8 +4,6 @@ import { sentryTest } from '../../../utils/fixtures'; import { envelopeRequestParser, shouldSkipTracingTest, waitForTransactionRequestOnUrl } from '../../../utils/helpers'; sentryTest('parses a string sample rate', async ({ getLocalTestUrl, page }) => { - // xxx test comment, remove before merging - if (shouldSkipTracingTest()) { sentryTest.skip(); } diff --git a/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/navigation/test.ts b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/navigation/test.ts index c61c2d2fad00..f24ca0507c66 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/navigation/test.ts +++ b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/navigation/test.ts @@ -8,7 +8,6 @@ import { } from '../../../../utils/helpers'; sentryTest('creates a new trace on each navigation', async ({ getLocalTestPath, page }) => { - // xxx test comment, remove before merging if (shouldSkipTracingTest()) { sentryTest.skip(); }