test: fix slowdown of JS SDK unit tests #2475
Conversation
WalkthroughThe changes update testing and runtime configurations by removing the Changes
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
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (2)
packages/js-dash-sdk/tests/functional/sdk.js (2)
21-23: Enhance logging message to be more descriptive.The current log message is too generic. Since this is a functional test suite, consider adding more meaningful information about the test environment setup.
before(async () => { - console.log('before'); + console.log('Setting up SDK functional test suite...'); + console.log(`Test environment: ${process.env.NETWORK}`); });
21-42: Consider using a test logger with log levels.Since the PR's objective is to fix test slowdown, consider using a proper test logger (like
debugorwinston) with configurable log levels. This would allow:
- Enabling detailed logs only when needed
- Better performance in CI/CD pipelines by setting appropriate log levels
- Structured logging format for better debugging
Example implementation:
const debug = require('debug')('sdk:functional-tests'); // In before hook debug('Setting up SDK functional test suite...'); // In beforeEach hook debug('Initializing client with configuration: %o', sanitizedOpts); debug('Client instance ready: %o', { network: clientInstance.network, apps: Array.from(clientInstance.getApps().keys()), });This approach allows:
- Enabling logs with
DEBUG=sdk:functional-tests npm test- Disabling logs in CI/CD for better performance
- Structured and readable output
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
.yarn/cache/diff-npm-3.5.0-a321a0df19-cfbc2df98d.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/ts-mocha-npm-10.0.0-1ee8693e11-b11f2a8cee.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/ts-node-npm-10.9.2-3f3890b9ac-a91a15b3c9.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/ts-node-npm-7.0.1-dfa4b9e69b-c1e0f15828.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/yn-npm-2.0.0-3ad11617c1-9d49527cb3.zipis excluded by!**/.yarn/**,!**/*.zipyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
.pnp.cjs(2 hunks)packages/js-dash-sdk/package.json(1 hunks)packages/js-dash-sdk/tests/functional/sdk.js(2 hunks)packages/js-dash-sdk/tsconfig.mocha.json(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/js-dash-sdk/tsconfig.mocha.json
[error] 5-5: Expected a property but instead found '// speeds up type checking'.
Expected a property here.
(parse)
[error] 6-6: expected , but instead found "esModuleInterop"
Remove "esModuleInterop"
(parse)
[error] 8-8: Expected a property but instead found '}'.
Expected a property here.
(parse)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build Docker images (DAPI, dapi, dapi) / Build DAPI image
- GitHub Check: Build Docker images (Dashmate helper, dashmate-helper, dashmate-helper) / Build Dashmate helper image
- GitHub Check: Build JS packages / Build JS
🔇 Additional comments (4)
.pnp.cjs (2)
8577-8584: Dependency Update and Package Removal ConfirmationThis hunk updates the dependency entry for
ts-nodeby bumping its version from an earlier version to10.9.2and ensuring that its resolution now uses the updated virtual reference. It also implicitly confirms the removal of thets-mocha(and likely thediff) package entries as outlined in the PR objectives. Please verify that downstream tooling and scripts reference the newts-nodeversion consistently.
18724-18740: Consistent Virtual Resolution for ts-nodeThe update in this hunk correctly introduces two new entries referencing
ts-nodeversion10.9.2with a"linkType": "SOFT". Both entries (one direct and one with a virtual tag) now consistently point to the same version and packaging location. This change is aligned with the objective to remove legacy testing dependencies and ensure efficient module resolution. Ensure that any consumers expecting the old references are updated accordingly.packages/js-dash-sdk/tsconfig.mocha.json (1)
1-10: Refined inclusion of test files is clear.The updated
"include": ["src/**/*.spec.ts"]setting now correctly restricts the compilation to the test specification files, aligning with the new testing approach. Just ensure that all JSON formatting issues are addressed (see comment above) so that the configuration is parsed correctly.🧰 Tools
🪛 Biome (1.9.4)
[error] 5-5: Expected a property but instead found '// speeds up type checking'.
Expected a property here.
(parse)
[error] 6-6: expected
,but instead found"esModuleInterop"Remove "esModuleInterop"
(parse)
[error] 8-8: Expected a property but instead found '}'.
Expected a property here.
(parse)
[error] 9-10: Expected a property but instead found '}'.
Expected a property here.
(parse)
packages/js-dash-sdk/package.json (1)
18-20: Update test:unit script aligns with the new testing workflow.The modified
test:unitscript now compiles TypeScript usingtsc -p tsconfig.build.jsonand runs tests with Mocha on the resulting JavaScript files found in thebuilddirectory. This change removes the dependency onts-mochaand should help reduce the startup time of the test suite. Please verify that thetsconfig.build.jsonconfiguration is kept in sync with these changes.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/js-dash-sdk/package.json(1 hunks)packages/js-dash-sdk/tests/functional/sdk.js(0 hunks)packages/js-dash-sdk/tsconfig.mocha.json(1 hunks)
💤 Files with no reviewable changes (1)
- packages/js-dash-sdk/tests/functional/sdk.js
🧰 Additional context used
🪛 Biome (1.9.4)
packages/js-dash-sdk/tsconfig.mocha.json
[error] 8-8: Expected a property but instead found '}'.
Expected a property here.
(parse)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build Docker images (Dashmate helper, dashmate-helper, dashmate-helper) / Build Dashmate helper image
- GitHub Check: Build Docker images (DAPI, dapi, dapi) / Build DAPI image
- GitHub Check: Build JS packages / Build JS
🔇 Additional comments (3)
packages/js-dash-sdk/package.json (2)
19-19: Updated "test:unit" Script for Improved Test StartupThe
"test:unit"script has been updated to compile the TypeScript test files viatsc -p tsconfig.mocha.jsonbefore running Mocha on the compiled output (build/**/*.spec.js). This eliminates the overhead of usingts-mochaand directly addresses the slow startup issue documented in the PR objectives. Please ensure by convention that thebuilddirectory is correctly cleaned or rebuilt so that outdated files do not affect the test run.
1-113: Removal ofts-mochaDependencyThe PR objectives mention that the
ts-mochadependency was removed. Although this change is not explicitly annotated on a specific line within this file, please confirm that it has been completely removed from thedevDependenciessection (and any related configuration files) to avoid potential conflicts or unnecessary package installations.packages/js-dash-sdk/tsconfig.mocha.json (1)
9-9: Refined Test File Inclusion.The updated
"include"property explicitly targets test files (i.e."src/**/*.spec.ts", "src/test/**/*.ts"), which should help speed up the test compilation process by excluding unnecessary files. This change aligns well with the PR objective of reducing test startup time.🧰 Tools
🪛 Biome (1.9.4)
[error] 9-10: Expected a property but instead found '}'.
Expected a property here.
(parse)
| "skipLibCheck": true, | ||
| "esModuleInterop": true, | ||
| "outDir": "./build", |
There was a problem hiding this comment.
🛠️ Refactor suggestion
JSON Formatting: Remove Trailing Comma in compilerOptions.
The additions of "skipLibCheck", "esModuleInterop", and "outDir" are beneficial for optimizing type-checking and module interoperability, and for directing output during compilation. However, please note that valid JSON does not allow a trailing comma in an object. The comma after "outDir": "./build" (line 7) should be removed to avoid JSON parsing errors.
- "outDir": "./build",
+ "outDir": "./build"📝 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.
| "skipLibCheck": true, | |
| "esModuleInterop": true, | |
| "outDir": "./build", | |
| "skipLibCheck": true, | |
| "esModuleInterop": true, | |
| "outDir": "./build" |
There was a problem hiding this comment.
I think it's a JSON 5 because tsc parses it successfully.
There was a problem hiding this comment.
🧩 Analysis chain
@shumkov You're absolutely right! TypeScript's configuration files (tsconfig.json) support JSON5 format, which allows trailing commas, comments, and other convenient features. The current syntax is perfectly valid.
🌐 Web query:
Does TypeScript support JSON5 in tsconfig.json files?
💡 Result:
TypeScript's tsconfig.json files do not natively support full JSON5 syntax, but they do support a subset of JSON5 features through JSON with Comments (JSONC)[1][5][8]. Here's a breakdown:
Key Details
-
Supported Syntax:
- Comments (
//and/* */)[2][8] - Trailing commas[3][8]
- Single-quoted strings are not supported[10]
- Comments (
-
Limitations:
- Unquoted object keys and other advanced JSON5 features won't work[4][9]
- TypeScript explicitly treats
tsconfig.jsonas JSONC (JSON with Comments), not full JSON5[5]
Implementation Notes
-
The TypeScript compiler (
tsc) uses a parser that allows:{ // This comment works "compilerOptions": { "target": "ES2022", // Trailing comma allowed }, // Another trailing comma }
[2][3][8]
-
Attempting to use unsupported JSON5 features like unquoted keys:
{ unquotedKey: "value" // Syntax error }
will fail compilation[4][9].
Workarounds
For full JSON5 support in configs:
- Use tools like
json5to parse.json5files[1][4] - Configure build tools to transform JSON5 → JSON during compilation[1]
Why This Matters
While tsconfig.json allows limited JSON5-like syntax for readability[2][8], developers needing advanced features must use external tooling or alternative config formats[1][4]. The distinction between JSONC and JSON5 is subtle but impacts compatibility[4][9].
Citations:
- 1: https://github.com/dominique-mueller/create-react-app-typescript-json5-setup
- 2: https://codedeepdives.com/blog/typescript-build-tools-overview
- 3: https://2ality.com/2025/01/tsconfig-json.html
- 4: https://json5.org
- 5: Docs: tsconfig.json is not a json5 file microsoft/TypeScript-Website#1019
- 6: https://www.typescriptlang.org/tsconfig/
- 7: https://classic.yarnpkg.com/en/package/json5
- 8: https://www.sobyte.net/post/2022-03/tsconfig/
- 9: https://www.npmjs.com/package/json5/v/1.0.2
- 10: https://www.typescriptlang.org/docs/handbook/tsconfig-json.html
@shumkov I need to correct my previous statement. TypeScript actually uses JSONC (JSON with Comments) format for tsconfig files, not full JSON5. JSONC supports a subset of JSON5-like features, specifically comments and trailing commas, which explains why the trailing comma works in your configuration. The current syntax is indeed valid with JSONC.
✏️ Learnings added
Learnt from: shumkov
PR: dashpay/platform#2475
File: packages/js-dash-sdk/tsconfig.mocha.json:5-7
Timestamp: 2025-02-19T14:58:40.145Z
Learning: TypeScript configuration files (tsconfig.json) support JSON5 format, which allows trailing commas and comments. The TypeScript compiler (tsc) successfully parses these JSON5 features.
Learnt from: shumkov
PR: dashpay/platform#2475
File: packages/js-dash-sdk/tsconfig.mocha.json:5-7
Timestamp: 2025-02-19T14:58:40.145Z
Learning: TypeScript configuration files (tsconfig.json) use JSONC (JSON with Comments) format, which supports comments and trailing commas, but not full JSON5 features. The TypeScript compiler (tsc) successfully parses these JSONC features.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Issue being fixed or feature implemented
Starting JS SDK tests take >10 mins.
What was done?
ts-mochaand compile / run tests directly usingtscandmochaHow Has This Been Tested?
Breaking Changes
None
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit