Skip to content

Commit 921e529

Browse files
mydeamydea
authored
ci: Streamline browser & node unit tests (getsentry#13307)
Instead of having to keep to separate lists of include/excludes, we now keep this in a single list and invert it when necessary. This way, we should no longer have problems where tests are either run multiple times, or not in the correct env - just add the test to the `browser` list in `ci-unit-tests.ts` to make sure it is not run in multiple node versions unnecessarily. I also added this step to the new package checklist.
1 parent 44641f0 commit 921e529

File tree

3 files changed

+70
-32
lines changed

3 files changed

+70
-32
lines changed

‎docs/new-sdk-release-checklist.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,8 +45,8 @@ differ slightly for other SDKs depending on how they are structured and how they
4545

4646
- [ ] Make sure `build.yml` CI script is correctly set up to cover tests for the new package
4747

48-
- [ ] Ensure dependent packages are correctly set for “Determine changed packages”
4948
- [ ] Ensure unit tests run correctly
49+
- [ ] If it is a browser SDK, add it to `BROWSER_TEST_PACKAGES` in `scripts/ci-unit-tests.ts`
5050

5151
- [ ] Make sure the file paths in the
5252
["Upload Artifacts" job](https://github.com/getsentry/sentry-javascript/blob/e5c1486eed236b878f2c49d6a04be86093816ac9/.github/workflows/build.yml#L314-L349)

‎package.json

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -35,10 +35,10 @@
3535
"test:unit": "lerna run --ignore \"@sentry-internal/{browser-integration-tests,e2e-tests,integration-shims,node-integration-tests,overhead-metrics}\" test:unit",
3636
"test:update-snapshots": "lerna run test:update-snapshots",
3737
"test:pr": "nx affected -t test --exclude \"@sentry-internal/{browser-integration-tests,e2e-tests,integration-shims,node-integration-tests,overhead-metrics}\"",
38-
"test:pr:browser": "yarn test:pr --exclude \"@sentry/{core,utils,opentelemetry,bun,deno,node,profiling-node,aws-serverless,google-cloud-serverless,nextjs,nestjs,astro,cloudflare,solidstart,nuxt,remix,gatsby,sveltekit,vercel-edge}\"",
39-
"test:pr:node": "ts-node ./scripts/node-unit-tests.ts --affected",
40-
"test:ci:browser": "lerna run test --ignore \"@sentry/{core,utils,opentelemetry,bun,deno,node,profiling-node,aws-serverless,google-cloud-serverless,nextjs,nestjs,astro,cloudflare,solidstart,nuxt,remix,gatsby,sveltekit,vercel-edge}\" --ignore \"@sentry-internal/{browser-integration-tests,e2e-tests,integration-shims,node-integration-tests,overhead-metrics}\"",
41-
"test:ci:node": "ts-node ./scripts/node-unit-tests.ts",
38+
"test:pr:browser": "UNIT_TEST_ENV=browser ts-node ./scripts/ci-unit-tests.ts --affected",
39+
"test:pr:node": "UNIT_TEST_ENV=node ts-node ./scripts/ci-unit-tests.ts --affected",
40+
"test:ci:browser": "UNIT_TEST_ENV=browser ts-node ./scripts/ci-unit-tests.ts",
41+
"test:ci:node": "UNIT_TEST_ENV=node ts-node ./scripts/ci-unit-tests.ts",
4242
"test:ci:bun": "lerna run test --scope @sentry/bun",
4343
"yalc:publish": "lerna run yalc:publish"
4444
},

‎scripts/node-unit-tests.ts renamed to ‎scripts/ci-unit-tests.ts

Lines changed: 65 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -1,17 +1,24 @@
11
import * as childProcess from 'child_process';
2+
import * as fs from 'fs';
3+
import * as path from 'path';
24

35
type NodeVersion = '14' | '16' | '18' | '20' | '21';
46

57
interface VersionConfig {
68
ignoredPackages: Array<`@${'sentry' | 'sentry-internal'}/${string}`>;
79
}
810

11+
const UNIT_TEST_ENV = process.env.UNIT_TEST_ENV as 'node' | 'browser' | undefined;
12+
913
const CURRENT_NODE_VERSION = process.version.replace('v', '').split('.')[0] as NodeVersion;
1014

1115
const RUN_AFFECTED = process.argv.includes('--affected');
1216

13-
const DEFAULT_SKIP_TESTS_PACKAGES = [
14-
'@sentry-internal/eslint-plugin-sdk',
17+
// These packages are tested separately in CI, so no need to run them here
18+
const DEFAULT_SKIP_PACKAGES = ['@sentry/profiling-node', '@sentry/bun', '@sentry/deno'];
19+
20+
// All other packages are run for multiple node versions
21+
const BROWSER_TEST_PACKAGES = [
1522
'@sentry/ember',
1623
'@sentry/browser',
1724
'@sentry/vue',
@@ -26,10 +33,9 @@ const DEFAULT_SKIP_TESTS_PACKAGES = [
2633
'@sentry-internal/replay-worker',
2734
'@sentry-internal/feedback',
2835
'@sentry/wasm',
29-
'@sentry/bun',
30-
'@sentry/deno',
3136
];
3237

38+
// These are Node-version specific tests that need to be skipped because of support
3339
const SKIP_TEST_PACKAGES: Record<NodeVersion, VersionConfig> = {
3440
'14': {
3541
ignoredPackages: [
@@ -40,6 +46,7 @@ const SKIP_TEST_PACKAGES: Record<NodeVersion, VersionConfig> = {
4046
'@sentry/astro',
4147
'@sentry/nuxt',
4248
'@sentry/nestjs',
49+
'@sentry-internal/eslint-plugin-sdk',
4350
],
4451
},
4552
'16': {
@@ -56,6 +63,50 @@ const SKIP_TEST_PACKAGES: Record<NodeVersion, VersionConfig> = {
5663
},
5764
};
5865

66+
function getAllPackages(): string[] {
67+
const { workspaces }: { workspaces: string[] } = JSON.parse(
68+
fs.readFileSync(path.join(process.cwd(), 'package.json'), 'utf-8'),
69+
);
70+
71+
return workspaces.map(workspacePath => {
72+
const { name }: { name: string } = JSON.parse(
73+
fs.readFileSync(path.join(process.cwd(), workspacePath, 'package.json'), 'utf-8'),
74+
);
75+
return name;
76+
});
77+
}
78+
79+
/**
80+
* Run the tests, accounting for compatibility problems in older versions of Node.
81+
*/
82+
function runTests(): void {
83+
const ignores = new Set<string>(DEFAULT_SKIP_PACKAGES);
84+
85+
const packages = getAllPackages();
86+
87+
if (UNIT_TEST_ENV === 'browser') {
88+
// Since we cannot "include" for affected mode, we instead exclude all other packages
89+
packages.forEach(pkg => {
90+
if (!BROWSER_TEST_PACKAGES.includes(pkg)) {
91+
ignores.add(pkg);
92+
}
93+
});
94+
} else if (UNIT_TEST_ENV === 'node') {
95+
BROWSER_TEST_PACKAGES.forEach(pkg => ignores.add(pkg));
96+
}
97+
98+
const versionConfig = SKIP_TEST_PACKAGES[CURRENT_NODE_VERSION];
99+
if (versionConfig) {
100+
versionConfig.ignoredPackages.forEach(dep => ignores.add(dep));
101+
}
102+
103+
if (RUN_AFFECTED) {
104+
runAffectedTests(ignores);
105+
} else {
106+
runAllTests(ignores);
107+
}
108+
}
109+
59110
/**
60111
* Run the given shell command, piping the shell process's `stdin`, `stdout`, and `stderr` to that of the current
61112
* process. Returns contents of `stdout`.
@@ -67,41 +118,28 @@ function run(cmd: string, options?: childProcess.ExecSyncOptions): void {
67118
/**
68119
* Run tests, ignoring the given packages
69120
*/
70-
function runWithIgnores(skipPackages: string[] = []): void {
71-
const ignoreFlags = skipPackages.map(dep => `--ignore="${dep}"`).join(' ');
121+
function runAllTests(ignorePackages: Set<string>): void {
122+
const ignoreFlags = Array.from(ignorePackages)
123+
.map(dep => `--ignore="${dep}"`)
124+
.join(' ');
125+
72126
run(`yarn test ${ignoreFlags}`);
73127
}
74128

75129
/**
76130
* Run affected tests, ignoring the given packages
77131
*/
78-
function runAffectedWithIgnores(skipPackages: string[] = []): void {
132+
function runAffectedTests(ignorePackages: Set<string>): void {
79133
const additionalArgs = process.argv
80134
.slice(2)
81135
.filter(arg => arg !== '--affected')
82136
.join(' ');
83-
const ignoreFlags = skipPackages.map(dep => `--exclude="${dep}"`).join(' ');
84-
run(`yarn test:pr ${ignoreFlags} ${additionalArgs}`);
85-
}
86-
87-
/**
88-
* Run the tests, accounting for compatibility problems in older versions of Node.
89-
*/
90-
function runTests(): void {
91-
const ignores = new Set<string>();
92-
93-
DEFAULT_SKIP_TESTS_PACKAGES.forEach(pkg => ignores.add(pkg));
94137

95-
const versionConfig = SKIP_TEST_PACKAGES[CURRENT_NODE_VERSION];
96-
if (versionConfig) {
97-
versionConfig.ignoredPackages.forEach(dep => ignores.add(dep));
98-
}
138+
const excludeFlags = Array.from(ignorePackages)
139+
.map(dep => `--exclude="${dep}"`)
140+
.join(' ');
99141

100-
if (RUN_AFFECTED) {
101-
runAffectedWithIgnores(Array.from(ignores));
102-
} else {
103-
runWithIgnores(Array.from(ignores));
104-
}
142+
run(`yarn test:pr ${excludeFlags} ${additionalArgs}`);
105143
}
106144

107145
runTests();

0 commit comments

Comments
 (0)