chore: improve test run speed #1018
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Important Auto Review SkippedReview was skipped due to path filters Files ignored due to path filters (2)
WalkthroughThe updates involve refining the project's configuration and testing setup. Changes include enhancing Changes
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.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
packages/testtools/package.jsonis excluded by:!**/*.jsonpackages/testtools/src/package.template.jsonis excluded by:!**/*.json
Files selected for processing (6)
- .gitignore (1 hunks)
- jest.config.ts (1 hunks)
- packages/testtools/src/schema.ts (4 hunks)
- test-setup.ts (1 hunks)
- tests/integration/jest.config.ts (1 hunks)
- tests/integration/tests/schema/todo.zmodel (1 hunks)
Files skipped from review due to trivial changes (1)
- .gitignore
Additional comments: 11
tests/integration/jest.config.ts (2)
- 1-1: Importing a base configuration from
../../jest.configis a good practice for maintaining consistency across different Jest configurations in the project. This approach helps in centralizing common configurations and reducing duplication.- 8-8: The addition of
setupFilesAfterEnv: ['./test-setup.ts']specifies a setup file to be executed after the test environment is set up but before the tests are run. This is a good practice for configuring or setting up testing frameworks that cannot be configured in the Jest configuration directly.jest.config.ts (3)
- 6-6: The addition of
import path from 'path';is necessary for usingpath.joinin the configuration. This is a good practice for handling file paths in a platform-independent manner.- 12-12: Adding a
globalSetupconfiguration with a path totest-setup.tsis a good practice. It allows executing a setup script before any of the tests are run, which can be used for setting up a test environment or performing global initializations.- 18-18: Using
path.join(__dirname, '.test/coverage')for thecoverageDirectoryensures that the coverage output directory is correctly resolved relative to the current file's directory. This is a more robust approach than hardcoding paths.test-setup.ts (1)
- 1-39: The
test-setup.tsfile introduces a comprehensive setup process for test scaffolding, including creating a scaffold directory, running initialization commands, and installing dependencies. This setup is crucial for ensuring a consistent and isolated environment for tests. The use ofpath.joinfor path handling,fs.existsSyncandfs.rmSyncfor directory cleanup, andexecSyncfor command execution are all appropriate choices for these tasks. Additionally, handling local dependencies installation is a good practice for testing in a monorepo setup or when local packages are involved. Overall, this setup script is well-structured and follows good practices for test environment preparation.tests/integration/tests/schema/todo.zmodel (1)
- 17-17: Commenting out
preserveTsFiles = truein theplugin zodsection may affect how TypeScript files are handled during testing, potentially leading to faster test execution by not preserving TypeScript files. It's important to ensure that this change aligns with the project's testing requirements and does not negatively impact the ability to debug tests or the accuracy of type checking.packages/testtools/src/schema.ts (4)
- 38-50: Adding a
try-catchblock around theexecSynccommand execution in therunfunction is a good practice for error handling. It ensures that any errors during command execution are caught and logged, providing better feedback and preventing unhandled exceptions.- 146-148: Renaming the variable from
roottoworkspaceRootfor clarity is a good practice. It makes the code more readable and understandable, especially in the context of monorepos or projects with complex directory structures.- 155-157: The addition of a file copy operation from the scaffold directory to the project root is a good practice for setting up a consistent test environment. It ensures that the necessary project structure and files are in place before running tests or generating schemas.
- 197-203: Modifying the
zenstack generatecommands to include additional options and specifyingNODE_PATHimproves the flexibility and correctness of schema generation. It allows for more precise control over the generation process and ensures that dependencies are correctly resolved.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| console.log('Execution took', Date.now() - start, 'ms', '-', cmd); | ||
| try { | ||
| const start = Date.now(); | ||
| execSync(cmd, { |
Check warning
Code scanning / CodeQL
Shell command built from environment values
packages/testtools/src/schema.ts
Outdated
Check warning
Code scanning / CodeQL
Unsafe shell command constructed from library input
packages/testtools/src/schema.ts
Outdated
Check warning
Code scanning / CodeQL
Unsafe shell command constructed from library input
There was a problem hiding this comment.
Review Status
Actionable comments generated: 5
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
package.jsonis excluded by:!**/*.json
Files selected for processing (7)
- .github/workflows/build-test.yml (1 hunks)
- CONTRIBUTING.md (1 hunks)
- packages/testtools/src/schema.ts (5 hunks)
- script/test-scaffold.ts (1 hunks)
- test-setup.ts (1 hunks)
- tests/integration/tests/cli/generate.test.ts (2 hunks)
- tests/integration/tests/cli/plugins.test.ts (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- test-setup.ts
Additional comments: 8
script/test-scaffold.ts (3)
- 5-9: Creating the scaffold directory is done without checking if it already exists, which is efficient due to the use of
fs.existsSyncfollowed byfs.rmSyncandfs.mkdirSync. This ensures the directory is fresh for each test run.- 11-18: The
runfunction correctly executes shell commands within thescaffoldPathdirectory and handles errors by logging them and re-throwing. This is a good practice for error handling in scripts, ensuring that errors do not go unnoticed.- 21-22: The use of
npm i --no-audit --no-fundflags during package installation is a good practice for speeding up npm installs in a CI/CD environment by skipping unnecessary checks..github/workflows/build-test.yml (1)
- 100-100: Adding
pnpm run test-scaffoldbeforepnpm run test-ciin the CI workflow is a logical step to ensure that the test scaffold is set up before running the main test suite. This change aligns with the PR's objective to improve test run speed by optimizing setup processes.CONTRIBUTING.md (1)
- 38-44: The addition of instructions for running
pnpm test-scaffoldin the contribution guide is clear and provides necessary guidance for contributors to set up the project for testing. Mentioning that this command only needs to be run once is helpful for avoiding confusion.tests/integration/tests/cli/generate.test.ts (1)
- 46-47: Replacing direct
execSynccalls withinstallPackagefor installing packages is a good practice. It abstracts the complexity of package installation, potentially handling edge cases and errors more gracefully. This change contributes to making the test setup more maintainable and readable.tests/integration/tests/cli/plugins.test.ts (1)
- 97-98: Using
installPackageinstead of directnpmcommands for installing dependencies in CLI Plugins Tests is an improvement. It standardizes the installation process and potentially provides better error handling and logging. This change enhances the maintainability and readability of the test setup.packages/testtools/src/schema.ts (1)
- 150-152: Changing the variable name from
roottoworkspaceRootimproves clarity and readability, making it easier to understand the purpose of the variable. This change aligns with best practices for naming conventions.
| } | ||
|
|
||
| export function installPackage(pkg: string, dev = false) { | ||
| run(`npm install ${dev ? '-D' : ''} --no-audit --no-fund ${pkg}`); |
Check warning
Code scanning / CodeQL
Unsafe shell command constructed from library input
| } | ||
|
|
||
| opt.extraDependencies?.forEach((dep) => { | ||
| console.log(`Installing dependency ${dep}`); |
Check warning
Code scanning / CodeQL
Unsafe shell command constructed from library input
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
zenstack generatecommands with additional options for better control..gitignoreto exclude.testdirectories.