-
-
Notifications
You must be signed in to change notification settings - Fork 89
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: improve test run speed #1018
Conversation
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
packages/testtools/package.json
is excluded by:!**/*.json
packages/testtools/src/package.template.json
is 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.config
is 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.join
in the configuration. This is a good practice for handling file paths in a platform-independent manner.- 12-12: Adding a
globalSetup
configuration with a path totest-setup.ts
is 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 thecoverageDirectory
ensures 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.ts
file 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.join
for path handling,fs.existsSync
andfs.rmSync
for directory cleanup, andexecSync
for 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 = true
in theplugin zod
section 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-catch
block around theexecSync
command execution in therun
function 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
root
toworkspaceRoot
for 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 generate
commands to include additional options and specifyingNODE_PATH
improves the flexibility and correctness of schema generation. It allows for more precise control over the generation process and ensures that dependencies are correctly resolved.
packages/testtools/src/schema.ts
Outdated
const outputArg = opt.output ? ` --output ${opt.output}` : ''; | ||
|
||
if (opt.customSchemaFilePath) { | ||
run(`npx zenstack generate --schema ${zmodelPath} --no-dependency-check${outputArg}`, { | ||
run(`npx zenstack generate --schema --no-version-check ${zmodelPath} --no-dependency-check${outputArg}`, { |
Check warning
Code scanning / CodeQL
Unsafe shell command constructed from library input Medium test
This string concatenation which depends on library input is later used in a shell command.
This string concatenation which depends on library input is later used in a shell command.
This string concatenation which depends on library input is later used in a shell command.
This string concatenation which depends on library input is later used in a shell command.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 5
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
package.json
is 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.existsSync
followed byfs.rmSync
andfs.mkdirSync
. This ensures the directory is fresh for each test run.- 11-18: The
run
function correctly executes shell commands within thescaffoldPath
directory 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-fund
flags 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-scaffold
beforepnpm run test-ci
in 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-scaffold
in 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
execSync
calls withinstallPackage
for 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
installPackage
instead of directnpm
commands 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
root
toworkspaceRoot
improves clarity and readability, making it easier to understand the purpose of the variable. This change aligns with best practices for naming conventions.
Summary by CodeRabbit
zenstack generate
commands with additional options for better control..gitignore
to exclude.test
directories.