Attempt at speeding up prisma-generator tests #1465
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis update improves the JavaScript testing setup by introducing utilities to handle project directory initialization and package management, specifically targeting Changes
Sequence DiagramssequenceDiagram
participant Developer
participant TestUtils
participant ProjectDir
participant PNPM
Developer->>TestUtils: Call preparePackageJson()
TestUtils-->>Developer: Returns modified package.json content
Developer->>TestUtils: Call initProjectDir()
TestUtils-->>ProjectDir: Create directory and write package.json
TestUtils-->>PNPM: Install dependencies
PNPM-->>TestUtils: Installation complete
TestUtils-->>Developer: Project directory ready
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 as PR comments)
Additionally, you can add CodeRabbit Configration 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: 4
Outside diff range and nitpick comments (2)
script/test-utils.ts (1)
36-44: TheexecCmdSyncfunction provides clear logging and error handling. Ensure that sensitive data is not logged in production environments.packages/schema/tests/generator/prisma-generator.test.ts (1)
Line range hint
4-4: Ensure that Node.js built-in modules are imported with thenode:protocol for consistency and clarity.- import fs from 'fs'; - import path from 'path'; + import fs from 'node:fs'; + import path from 'node:path';Also applies to: 5-5
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- .gitignore (1 hunks)
- packages/schema/tests/generator/prisma-generator.test.ts (1 hunks)
- script/test-utils.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- .gitignore
Additional Context Used
Biome (6)
packages/schema/tests/generator/prisma-generator.test.ts (5)
4-4: A Node.js builtin module should be imported with the node: protocol.
5-5: A Node.js builtin module should be imported with the node: protocol.
21-21: The computed expression can be simplified without the use of a string literal.
23-23: Do not use template literals if interpolation and special-character handling are not needed.
221-221: Do not use template literals if interpolation and special-character handling are not needed.
script/test-utils.ts (1)
46-46: This type annotation is trivially inferred from its initialization.
Additional comments not posted (3)
script/test-utils.ts (1)
6-10: The constants for paths and file contents are well-defined and follow best practices for maintainability and reusability.packages/schema/tests/generator/prisma-generator.test.ts (2)
10-10: Correct usage of imported utility functions fromscript/test-utils.ts. This ensures consistency and reusability across tests.
32-32: TheinitProjectDirfunction is used correctly in thebeforeEachhook to ensure a clean environment for each test. Good practice!
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (1)
packages/schema/tests/generator/prisma-generator.test.ts (1)
Line range hint
4-5: Update import statements to use thenode:protocol for built-in modules.- import fs from 'fs'; - import path from 'path'; + import fs from 'node:fs'; + import path from 'node:path';
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- .gitignore (1 hunks)
- packages/schema/tests/generator/prisma-generator.test.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- .gitignore
Additional Context Used
Biome (4)
packages/schema/tests/generator/prisma-generator.test.ts (4)
4-4: A Node.js builtin module should be imported with the node: protocol.
5-5: A Node.js builtin module should be imported with the node: protocol.
21-21: The computed expression can be simplified without the use of a string literal.
220-220: Do not use template literals if interpolation and special-character handling are not needed.
Additional comments not posted (1)
packages/schema/tests/generator/prisma-generator.test.ts (1)
Line range hint
36-36: The test cases are well-structured and cover the necessary functionality effectively.
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (3)
packages/schema/tests/generator/prisma-generator.test.ts (3)
Line range hint
4-4: Update the import statement to use thenode:protocol for built-in modules.- import fs from 'fs'; + import fs from 'node:fs';
Line range hint
5-5: Update the import statement to use thenode:protocol for built-in modules.- import path from 'path'; + import path from 'node:path';
Line range hint
220-220: Simplify the use of template literals where simple strings would suffice.- expect(content).toContain('provider = "postgresql"'); + expect(content).toContain('provider = postgresql');
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- packages/schema/tests/generator/prisma-generator.test.ts (1 hunks)
- script/test-utils.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- script/test-utils.ts
Additional context used
Biome
packages/schema/tests/generator/prisma-generator.test.ts
[error] 4-4: A Node.js builtin module should be imported with the node: protocol.
[error] 5-5: A Node.js builtin module should be imported with the node: protocol.
[error] 21-21: The computed expression can be simplified without the use of a string literal.
[error] 220-220: Do not use template literals if interpolation and special-character handling are not needed.
|
Hi @WimTibackx, I'm sorry for the late response. Thanks for working on improving test speed. I've been bothered by it for quite a while but have been procrastinating on it 😅. I think using a shared pnpm store is a great idea, which should make the installation process significantly faster, especially for running locally. From another perspective, I'm also wondering if we need to scaffold a npm project for each test at all. The tests were made this way mainly because Prisma and ZenStack both do code generation into node_modules and using disposable projects is the simplest way to isolate. However, since both Prisma and ZenStack allow specifying where to output the generated code, we can probably only emit them into some temp dir while not having a full-scale npm project at all. What do you think about this direction? It's gonna be a bigger change though. |
…ecific store once, then offline-installing from that store for each test going forward
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
WimTibackx
force-pushed
the
test-speed-experiments
branch
from
08b9402 to
11068cb
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- .gitignore (1 hunks)
- packages/schema/tests/generator/prisma-generator.test.ts (1 hunks)
- script/test-utils.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- .gitignore
Additional comments not posted (6)
script/test-utils.ts (2)
1-4: Imports are well-organized and essential for the functionality provided by this module.
6-10: Constants are clearly defined and well-documented, ensuring easy maintenance and understanding.packages/schema/tests/generator/prisma-generator.test.ts (4)
10-10: Correct usage of imported functions fromtest-utilsto facilitate testing setup.
31-31: Efficient use ofinitProjectDirwithinbeforeEachensures a clean setup for each test case.
28-28: The logging statement inbeforeEachcould be simplified for consistency and readability.- console.log('Project dir: ', r.name); + console.log('Project directory:', r.name);Likely invalid or redundant comment.
19-23: ThebeforeAllhook is used effectively to prepare the environment for tests. Consider simplifying the logging statement to enhance readability.- console.log(`Got my packageJsonContents`); + console.log('Package JSON contents prepared');Likely invalid or redundant comment.
script/test-utils.ts
Outdated
| export function initProjectDir(projectDir: string, packageJsonContents: string, offline = true) { | ||
| try { | ||
| if (!fs.existsSync(projectDir)) { | ||
| fs.mkdirSync(projectDir, { recursive: true }); | ||
| } | ||
| fs.writeFileSync(path.join(projectDir, PACKAGE_JSON_FILE), packageJsonContents, { flag: 'w+' }); | ||
| fs.writeFileSync(path.join(projectDir, NPM_RC_FILE), NPM_RC_CONTENTS, { flag: 'w+' }); | ||
| } catch (e) { | ||
| console.error(`Failed to set up project dir in ${projectDir}`); | ||
| throw e; | ||
| } | ||
|
|
||
| try { | ||
| execSync(`pnpm install ${offline ? '--offline ' : ''}--ignore-workspace`, {cwd: projectDir, stdio: 'ignore'}); | ||
| } catch (e) { | ||
| console.error(`Failed to initialize project dependencies in ${projectDir}${offline ? '(offline mode)' : '(online mode)'}`); | ||
| throw e; | ||
| } | ||
| } |
There was a problem hiding this comment.
The initProjectDir function effectively sets up the project directory and handles potential errors robustly. However, the error messages could be made more descriptive to aid debugging.
- console.error(`Failed to set up project dir in ${projectDir}`);
+ console.error(`Failed to set up project directory in ${projectDir}: ${e.message}`);
- console.error(`Failed to initialize project dependencies in ${projectDir}${offline ? '(offline mode)' : '(online mode)'}`);
+ console.error(`Failed to initialize project dependencies in ${projectDir}${offline ? '(offline mode)' : '(online mode)'}: ${e.message}`);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.
| export function initProjectDir(projectDir: string, packageJsonContents: string, offline = true) { | |
| try { | |
| if (!fs.existsSync(projectDir)) { | |
| fs.mkdirSync(projectDir, { recursive: true }); | |
| } | |
| fs.writeFileSync(path.join(projectDir, PACKAGE_JSON_FILE), packageJsonContents, { flag: 'w+' }); | |
| fs.writeFileSync(path.join(projectDir, NPM_RC_FILE), NPM_RC_CONTENTS, { flag: 'w+' }); | |
| } catch (e) { | |
| console.error(`Failed to set up project dir in ${projectDir}`); | |
| throw e; | |
| } | |
| try { | |
| execSync(`pnpm install ${offline ? '--offline ' : ''}--ignore-workspace`, {cwd: projectDir, stdio: 'ignore'}); | |
| } catch (e) { | |
| console.error(`Failed to initialize project dependencies in ${projectDir}${offline ? '(offline mode)' : '(online mode)'}`); | |
| throw e; | |
| } | |
| } | |
| export function initProjectDir(projectDir: string, packageJsonContents: string, offline = true) { | |
| try { | |
| if (!fs.existsSync(projectDir)) { | |
| fs.mkdirSync(projectDir, { recursive: true }); | |
| } | |
| fs.writeFileSync(path.join(projectDir, PACKAGE_JSON_FILE), packageJsonContents, { flag: 'w+' }); | |
| fs.writeFileSync(path.join(projectDir, NPM_RC_FILE), NPM_RC_CONTENTS, { flag: 'w+' }); | |
| } catch (e) { | |
| console.error(`Failed to set up project directory in ${projectDir}: ${e.message}`); | |
| throw e; | |
| } | |
| try { | |
| execSync(`pnpm install ${offline ? '--offline ' : ''}--ignore-workspace`, {cwd: projectDir, stdio: 'ignore'}); | |
| } catch (e) { | |
| console.error(`Failed to initialize project dependencies in ${projectDir}${offline ? '(offline mode)' : '(online mode)'}: ${e.message}`); | |
| throw e; | |
| } | |
| } |
script/test-utils.ts
Outdated
| export function preparePackageJson(dependencies: {[key: string]: string} = {}, devDependencies: {[key: string]: string} = {}): string { | ||
| // Given that this is a loose file included from elsewhere, I couldn't rely on the tmp package here and had to go with built-in node functions. I saw no significant downsides in this case, versus the upside in developer experience of not needing to do a build step when changing these utils. | ||
| const tmpDir = fs.mkdtempSync(path.join(tmpdir(), 'zenstack-test-')); | ||
| console.log(`Loading dependencies into store via temp dir ${tmpDir}`); | ||
| try { | ||
| const packageJsonContents = | ||
| `{ | ||
| "name":"test-project", | ||
| "version":"1.0.0", | ||
| "dependencies": { | ||
| ${Object.entries(dependencies).map(([k, v]) => `"${k}": "${v}"`).join(',\n')} | ||
| }, | ||
| "devDependencies": { | ||
| ${Object.entries(devDependencies).map(([k, v]) => `"${k}": "${v}"`).join(',\n')} | ||
| } | ||
| }`; | ||
|
|
||
| // I considered doing a `pnpm store add` here instead of a plain install. While that worked, I decided against it in the end because it's a secondary way of processing the dependencies and I didn't see a significant downside to just installing and throwing the local project away right after. | ||
| initProjectDir(tmpDir, packageJsonContents, false); | ||
|
|
||
| return packageJsonContents; | ||
| } finally { | ||
| fs.rmSync(tmpDir, {recursive: true, force: true}); | ||
| } | ||
| } |
There was a problem hiding this comment.
The preparePackageJson function is well-structured for generating a temporary directory and handling dependencies. Consider adding a comment explaining the decision to use pnpm install instead of pnpm store add for future maintainers.
+ // Using `pnpm install` for simplicity and because it's the primary way of handling dependencies.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.
| export function preparePackageJson(dependencies: {[key: string]: string} = {}, devDependencies: {[key: string]: string} = {}): string { | |
| // Given that this is a loose file included from elsewhere, I couldn't rely on the tmp package here and had to go with built-in node functions. I saw no significant downsides in this case, versus the upside in developer experience of not needing to do a build step when changing these utils. | |
| const tmpDir = fs.mkdtempSync(path.join(tmpdir(), 'zenstack-test-')); | |
| console.log(`Loading dependencies into store via temp dir ${tmpDir}`); | |
| try { | |
| const packageJsonContents = | |
| `{ | |
| "name":"test-project", | |
| "version":"1.0.0", | |
| "dependencies": { | |
| ${Object.entries(dependencies).map(([k, v]) => `"${k}": "${v}"`).join(',\n')} | |
| }, | |
| "devDependencies": { | |
| ${Object.entries(devDependencies).map(([k, v]) => `"${k}": "${v}"`).join(',\n')} | |
| } | |
| }`; | |
| // I considered doing a `pnpm store add` here instead of a plain install. While that worked, I decided against it in the end because it's a secondary way of processing the dependencies and I didn't see a significant downside to just installing and throwing the local project away right after. | |
| initProjectDir(tmpDir, packageJsonContents, false); | |
| return packageJsonContents; | |
| } finally { | |
| fs.rmSync(tmpDir, {recursive: true, force: true}); | |
| } | |
| } | |
| export function preparePackageJson(dependencies: {[key: string]: string} = {}, devDependencies: {[key: string]: string} = {}): string { | |
| // Given that this is a loose file included from elsewhere, I couldn't rely on the tmp package here and had to go with built-in node functions. I saw no significant downsides in this case, versus the upside in developer experience of not needing to do a build step when changing these utils. | |
| const tmpDir = fs.mkdtempSync(path.join(tmpdir(), 'zenstack-test-')); | |
| console.log(`Loading dependencies into store via temp dir ${tmpDir}`); | |
| try { | |
| const packageJsonContents = | |
| `{ | |
| "name":"test-project", | |
| "version":"1.0.0", | |
| "dependencies": { | |
| ${Object.entries(dependencies).map(([k, v]) => `"${k}": "${v}"`).join(',\n')} | |
| }, | |
| "devDependencies": { | |
| ${Object.entries(devDependencies).map(([k, v]) => `"${k}": "${v}"`).join(',\n')} | |
| } | |
| }`; | |
| // Using `pnpm install` for simplicity and because it's the primary way of handling dependencies. | |
| // I considered doing a `pnpm store add` here instead of a plain install. While that worked, I decided against it in the end because it's a secondary way of processing the dependencies and I didn't see a significant downside to just installing and throwing the local project away right after. | |
| initProjectDir(tmpDir, packageJsonContents, false); | |
| return packageJsonContents; | |
| } finally { | |
| fs.rmSync(tmpDir, {recursive: true, force: true}); | |
| } | |
| } |
|
Hi @ymc9! It's about time I respond to this. Sorry for the delayed reply, I got a bit wrapped up in work and life for a bit.
I've been looking into this last weekend, but it requires some further tinkering. My original proof of concept probably used the easiest set of dependencies, unfortunately :) . The bigger dependency trees and the post-install scripts require some further work for this PR. I'm taking another shot at it today, will keep you posted.
That's a fair point indeed. As long as there's one full-on npm project integration test per module / variant, having other test cases run based on results emitting to a temp dir could suffice indeed. For now, I'll see if I can universally apply the pnpm store approach, as that should already help a fair bit. |
…for consistency and added reuse possibilities. removed pnpm store initialization from the scaffolding process for now, because it's slightly easier dependency wise and because doing this in scaffold is something I want to revisit anyway. I'd like to end up in a system where we can include a large superset of the dependencies in play. Not sure if that's attainable at this point. Given that we now don't run pnpm store initialization in test-scaffold, this means the performance hit will happen on (multiple?) first tests. This is not acceptable for the end result, but as an intermediate state in this PR it's good enough. Note as well that the exact implementation of what is now pnpm-project.ts changed a bit: it now uses the tmp package for consistency, optionally includes some standard dependencies, and includes workspace overrides (because the pnpm behavior with regards to deep dependencies is different fron npm).
allow splitting of dependencies and devDependencies
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| if (opt.customSchemaFilePath) { | ||
| run(`npx zenstack generate --no-version-check --schema ${zmodelPath} --no-dependency-check${outputArg}`, { | ||
| NODE_PATH: './node_modules', | ||
| run(`pnpm exec zenstack generate --no-version-check --schema ${zmodelPath} --no-dependency-check${outputArg}`, { |
Check warning
Code scanning / CodeQL
Unsafe shell command constructed from library input Medium test
| }); | ||
| } else { | ||
| run(`npx zenstack generate --no-version-check --no-dependency-check${outputArg}`, { | ||
| NODE_PATH: './node_modules', | ||
| run(`pnpm exec zenstack generate --no-version-check --no-dependency-check${outputArg}`, { |
Check warning
Code scanning / CodeQL
Unsafe shell command constructed from library input Medium test
…icated between schema tests and the testtools package. Might want to look for a better solution in the future.
|
Closing for now. Will pick it up again as necessary. |
In working on #1458 I noticed that some tests took me a while to execute. A lot of it seems to be centered around npm installs, so I picked out the prisma generator tests to experiment on.
As a baseline, current dev tests for that file took around 400 seconds on my system; with the npm installs taking up between 20 and 40 seconds.
With the
--no-progress --no-audit --no-fundflags, total time for this file reduced to 300 seconds on my system.I then looked at adding a test project directory, installing that in test-scaffold and copying from there each test. That reduced execution time to about 100 seconds on my system.
Given that this would seperate definition of the test context (which dependencies etc) from test execution, I wanted to try and avoid that if I could; so I looked at pnpm store. That is the version I finally submitted in this PR:
pnpm install --offlineto quickly install from the store.On my system this executes at around 55 seconds with an empty store, and at around 45 seconds when I've already populated the store earlier.
To me, this approach seems like a nice middle ground:
Happy to hear your thoughts on this! Is this an approach worth taking? Do you see downsides I'm missing here? Are there other factors to consider? Do you see room for further improvement?