Replica test runner #246
Replica test runner #246
Conversation
WalkthroughThe changes encompass updates across various files in the CLI application, introducing new functionalities, enhancing configuration management, and refining type handling. Key modifications include the addition of a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Replica
participant Canister
User->>CLI: Run test command
CLI->>Replica: Check configuration
Replica->>Canister: Start canister
Canister-->>Replica: Canister started
CLI->>Canister: Execute tests
Canister-->>CLI: Return test results
CLI-->>User: Display results
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: 4
Outside diff range, codebase verification and nitpick comments (3)
cli/package.json (1)
77-77: Discrepancy inpic-icversion across application componentsThe
pic-icpackage version has been updated to0.5.1in the CLI part of the application, but the frontend still references version0.3.2. This inconsistency might lead to issues if both parts of the application rely on different versions of the same package. Please verify and align the package versions across the application components to ensure compatibility and stability.
cli/package.jsonandcli/package-lock.json:pic-icversion0.5.1frontend/package-lock.json:pic-icversion0.3.2Analysis chain
Verify the version update for
pic-ic.Ensure that the version update aligns with the requirements and dependencies of the application and does not introduce any issues.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the version update for `pic-ic`. # Test: Search for the version update. Expect: Correct alignment with application requirements. rg --type json -A 5 $'pic-ic'Length of output: 2010
cli/commands/bench.ts (1)
Line range hint
52-150: Refinement of thebenchfunctionThe
benchfunction has been refined with the initialization of theconfigvariable at the beginning, which is a good practice for centralizing configuration management. This change allows the function to dynamically determine thereplicaoption based on the configuration, enhancing flexibility and maintainability.However, the modification of the cleanup process by commenting out the removal of the
benchDirdirectory could lead to residual files being left behind. This might affect the cleanliness of the environment for subsequent runs and could potentially lead to inconsistent benchmark results.It is recommended to either reinstate the cleanup process or implement a more controlled cleanup mechanism that ensures the environment is clean for each run.
Would you like to discuss potential approaches to improve the cleanup process?
cli/commands/test/test.ts (1)
Update function call to match new signature
The
testWithReporterfunction call incli/commands/publish.tsdoes not include thewatchparameter, which is part of the updated function signature. This discrepancy could lead to potential issues if thewatchparameter is necessary for correct functionality.
- File:
cli/commands/publish.ts
- Line:
await testWithReporter(reporter, '', 'interpreter', config.toolchain?.['pocket-ic'] ? 'pocket-ic' : 'dfx');- Action: Update the function call to include the
watchparameter.Analysis chain
Line range hint
143-305: Significant enhancements to testing capabilities.The update to the
testWithReporterfunction to includedefaultMode,replicaType, andwatchparameters, along with support for the newreplicamode, significantly enhances the testing framework's capabilities. These changes allow for more robust testing scenarios and better integration with the replica environment.The code changes are approved.
Run the following script to verify the function usage:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `testWithReporter` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type typescript -A 5 $'testWithReporter'Length of output: 79
Script:
#!/bin/bash # Description: Verify all function calls to `testWithReporter` match the new signature. # Test: Search for the function usage in .ts files. Expect: Only occurrences of the new signature. rg 'testWithReporter' --glob '*.ts' -A 5Length of output: 1594
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
cli/package-lock.jsonis excluded by!**/package-lock.json
Files selected for processing (20)
- cli/api/actors.ts (1 hunks)
- cli/cli.ts (3 hunks)
- cli/commands/bench-replica.ts (5 hunks)
- cli/commands/bench.ts (4 hunks)
- cli/commands/publish.ts (1 hunks)
- cli/commands/replica.ts (1 hunks)
- cli/commands/test/mmf1.ts (2 hunks)
- cli/commands/test/reporters/compact-reporter.ts (2 hunks)
- cli/commands/test/reporters/files-reporter.ts (3 hunks)
- cli/commands/test/reporters/reporter.ts (1 hunks)
- cli/commands/test/reporters/silent-reporter.ts (2 hunks)
- cli/commands/test/reporters/verbose-reporter.ts (4 hunks)
- cli/commands/test/test.ts (10 hunks)
- cli/mops.ts (1 hunks)
- cli/package.json (1 hunks)
- cli/types.ts (1 hunks)
- mops.toml (1 hunks)
- test/storage-actor.test.mo (1 hunks)
- test/storage-actor22222.test.mo (1 hunks)
- test/storage-actor33333.test.mo (1 hunks)
Files skipped from review due to trivial changes (2)
- cli/api/actors.ts
- cli/commands/test/mmf1.ts
Additional comments not posted (27)
cli/commands/test/reporters/reporter.ts (1)
6-6: Enhanced flexibility and type safety inaddRunmethod.The changes to the
addRunmethod in theReporterinterface improve flexibility and maintainability by introducing a more structured approach to handling test modes. Ensure that all implementations of this interface are updated to accommodate these changes.The code changes are approved.
Run the following script to verify the implementation across the codebase:
Verification successful
All implementations of
addRunmethod updated to match new signature.The
addRunmethod in theReporterinterface has been updated across the codebase to include themodeparameter. Theprogressiveparameter is optional and is used where necessary. Ensure that any future changes to the method signature are consistently applied across all implementations.
cli/commands/test/reporters/silent-reporter.tscli/commands/test/reporters/verbose-reporter.tscli/commands/test/reporters/files-reporter.tscli/commands/test/reporters/compact-reporter.tsScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all implementations of the `addRun` method in the `Reporter` interface match the new signature. # Test: Search for the method implementation. Expect: Only occurrences of the new signature. rg --type typescript -A 5 $'addRun'Length of output: 69
Script:
#!/bin/bash # Description: Verify all implementations of the `addRun` method in the `Reporter` interface match the new signature. # Test: Search for the method implementation. Expect: Only occurrences of the new signature. rg --type ts -A 5 $'addRun'Length of output: 2528
mops.toml (1)
21-21: Significant version update forpocket-ic.The update from
1.0.0to4.0.0for thepocket-icdependency is significant. It's crucial to verify the impact of this change on the project, especially looking for any breaking changes or new features that might affect existing functionalities.The code changes are approved.
Run the following script to verify the impact of the version change:
Verification successful
Verification successful for
pocket-icversion update.The codebase includes explicit checks and configurations for
pocket-icversion4.0.0, indicating that the update is intentional and well-integrated. No further issues were found regarding this version change.
- The code in
cli/cli.ts,cli/commands/publish.ts,cli/commands/bench.ts, and other files shows controlled usage ofpocket-ic.- Compatibility checks for version
4.0.0are present incli/commands/replica.tsandcli/commands/bench-replica.ts.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of the `pocket-ic` version update on the project. # Test: Search for the usage of `pocket-ic` in the project. Expect: No breaking changes. rg --type toml -A 5 $'pocket-ic'Length of output: 61
Script:
#!/bin/bash # Search for the usage of `pocket-ic` in the codebase to determine its impact. rg 'pocket-ic' -A 5Length of output: 14457
cli/types.ts (1)
45-45: New typeTestModeenhances type safety.The addition of the
TestModetype, which can take values 'interpreter', 'wasi', or 'replica', enhances type safety and clarity in handling different testing scenarios. Ensure that this new type is properly utilized across the codebase where relevant.The code changes are approved.
Run the following script to verify the usage of the
TestModetype across the codebase:Verification successful
TestModetype is properly utilized across the codebase.The
TestModetype is effectively integrated into the codebase, being used in various files such ascli/commands/test/test.tsand multiple reporter files. It is employed in function parameters and method calls, ensuring type safety and clarity in handling different testing scenarios.
cli/commands/test/test.ts: Used in function parameters.cli/commands/test/reporters/verbose-reporter.ts: Used in method calls.cli/commands/test/reporters/silent-reporter.ts: Used in method calls.cli/commands/test/reporters/files-reporter.ts: Used in method calls.cli/commands/test/reporters/compact-reporter.ts: Used in method calls.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the `TestMode` type in the project. # Test: Search for the usage of `TestMode` in the project. Expect: Proper utilization. rg --type typescript -A 5 $'TestMode'Length of output: 71
Script:
#!/bin/bash # Description: Verify the usage of the `TestMode` type in the project. # Test: Search for the usage of `TestMode` in the project. Expect: Proper utilization. rg -A 5 'TestMode'Length of output: 6298
cli/commands/test/reporters/silent-reporter.ts (2)
5-5: Import statement forTestModeis correctly added.The import is necessary for the new type
TestModeto be recognized within this file.
17-17: Method signature updated to useTestMode.The update from a boolean to a more descriptive type enhances clarity and type safety. Ensure that all calls to this method across the codebase are updated to reflect this change.
The method changes are approved.
Run the following script to verify the method usage:
Verification successful
Verification successful:
addRunmethod usage is consistent with the new signature.The
addRunmethod in theSilentReporterclass has been updated to include the_modeparameter, and its usage in the codebase reflects this change. The call incli/commands/test/test.tsmatches the new signature, ensuring consistency and correctness. No further action is required.
cli/commands/test/test.ts: Call toaddRunincludes themodeparameter.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `addRun` in `SilentReporter` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type typescript -A 5 $'addRun' 'cli/commands/test/reporters/silent-reporter.ts'Length of output: 116
Script:
#!/bin/bash # Description: Verify all function calls to `addRun` in `SilentReporter` match the new signature. # Test: Search for the function usage across the entire codebase. rg -A 5 'addRun'Length of output: 2518
cli/commands/test/reporters/files-reporter.ts (2)
5-5: Import statement forTestModeis correctly added.The import is necessary for the new type
TestModeto be recognized within this file.
19-19: Method signature updated to useTestMode, and console output logic adjusted.The update from a boolean to a more descriptive type enhances clarity and type safety. The console output logic has been appropriately adjusted to reflect the new
modeparameter. Ensure that all calls to this method across the codebase are updated to reflect this change.The method changes are approved.
Run the following script to verify the method usage:
Also applies to: 31-31
Verification successful
Method signature update verified across the codebase.
The
addRunmethod's signature update to include themodeparameter has been consistently applied across the codebase. The usage intest.tsand other reporter files confirms that the change has been properly propagated. No further action is needed.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `addRun` in `FilesReporter` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type typescript -A 5 $'addRun' 'cli/commands/test/reporters/files-reporter.ts'Length of output: 115
Script:
#!/bin/bash # Description: Verify all function calls to `addRun` in `FilesReporter` match the new signature. # Test: Search for the function usage across the codebase. rg 'addRun' -A 5Length of output: 2518
cli/commands/test/reporters/verbose-reporter.ts (3)
5-5: Import statement forTestModeis correctly added.The import is necessary for the new type
TestModeto be recognized within this file.
61-64: New method_printStartcorrectly encapsulates start printing logic.The addition of the
_printStartmethod improves code clarity and maintainability by separating concerns. It provides a more descriptive way to handle test modes during the start of a test run.The new method is approved.
23-23: Method signature updated to useTestModeandprogressive, and logic adjusted.The update from a boolean to a more descriptive type enhances clarity and type safety. The addition of the
progressiveparameter allows for more flexible handling of test reporting. The method conditionally calls the new_printStartmethod based on theprogressiveflag, improving code clarity by separating concerns. Ensure that all calls to this method across the codebase are updated to reflect this change.The method changes are approved.
Run the following script to verify the method usage:
Also applies to: 36-38
Verification successful
Verification successful:
addRunmethod usage is consistent with the new signature.The
addRunmethod inVerboseReporterhas been correctly updated to include themodeandprogressiveparameters. The usage incli/commands/test/test.tsaligns with the new signature, with theprogressiveparameter defaulting tofalse. Other reporters have their own implementations, which do not require theprogressiveparameter. No further action is needed.
cli/commands/test/test.ts: Usage matches the new signature.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `addRun` in `VerboseReporter` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type typescript -A 5 $'addRun' 'cli/commands/test/reporters/verbose-reporter.ts'Length of output: 117
Script:
#!/bin/bash # Description: Verify all function calls to `addRun` in `VerboseReporter` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type ts -A 5 'addRun'Length of output: 2528
cli/commands/test/reporters/compact-reporter.ts (2)
6-6: Approved: Import ofTestMode.The import enhances type safety and clarity in method signatures.
27-27: Approved: Method signature change inaddRun.The change from a boolean to
TestModeenhances clarity and type safety. Ensure all calls to this method are updated to pass the correctTestModevalue.Run the following script to verify the method usage:
Verification successful
Verified: Method signature change in
addRunis consistently applied.The transition from a boolean to
TestModehas been correctly implemented across all usages of theaddRunmethod in the codebase. No further action is needed.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `addRun` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type typescript -A 5 $'addRun'Length of output: 69
Script:
#!/bin/bash # Description: Verify all function calls to `addRun` match the new signature. # Test: Search for the function usage in TypeScript files. rg 'addRun' --glob '*.ts' -A 5Length of output: 2532
test/storage-actor.test.mo (1)
1-7: Approved: Import statements.The imports are correctly placed and used appropriately in the tests.
test/storage-actor22222.test.mo (1)
1-7: Approved: Import statements.The imports are correctly placed and used appropriately in the tests.
test/storage-actor33333.test.mo (3)
1-7: Import statements are appropriate.The import statements are correctly aligned with the functionality required in the tests.
18-98: Review the test cases for completeness and correctness.The test cases cover a wide range of scenarios. Ensure that all edge cases are considered and that the tests are correctly asserting the expected outcomes.
Verification successful
Test cases are complete and correct.
The test cases in
test/storage-actor33333.test.mocover a wide range of scenarios, including both successful operations and error handling. The assertions are correctly verifying the expected outcomes. No issues were found with the completeness or correctness of the test cases.
- The tests include scenarios for starting, finishing, and handling uploads and downloads.
- Assertions check both successful and erroneous conditions.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the completeness and correctness of test cases. # Test: Search for the test cases and their assertions. Expect: Comprehensive coverage and correct assertions. rg --type motoko -A 5 $'test'Length of output: 59
Script:
#!/bin/bash # Find all Motoko files and search for test cases within them. fd -e mo | xargs rg -A 5 'test'Length of output: 49783
9-11: Verify the usage ofExperimentalCycles.add.Ensure that the addition of cycles is appropriate and does not lead to unintended consequences in the test environment.
cli/commands/bench-replica.ts (4)
6-6: Verify new imports and properties forPocketIcServer.Ensure that the new imports and properties are correctly implemented and do not introduce any side effects or dependencies that could affect the stability of the application.
Also applies to: 15-15
Verification successful
Imports and properties for
PocketIcServerare correctly implemented.The
PocketIcServeris consistently used across the codebase without introducing side effects. The imports and properties are correctly implemented in the filescli/commands/bench-replica.tsandcli/commands/replica.ts.
cli/commands/bench-replica.tscli/commands/replica.tsScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the new imports and properties related to `PocketIcServer`. # Test: Search for the usage of `PocketIcServer`. Expect: Proper implementation without side effects. rg --type typescript -A 5 $'PocketIcServer'Length of output: 77
Script:
#!/bin/bash # Description: Verify the new imports and properties related to `PocketIcServer`. # Test: Search for the usage of `PocketIcServer`. Expect: Proper implementation without side effects. rg --type ts -A 5 $'PocketIcServer'Length of output: 2098
35-36: Verify the updated version check forpocket-ic.Ensure that the updated version check aligns with the requirements and dependencies of the application and does not exclude necessary versions inadvertently.
Verification successful
Version check for
pocket-icis verified and consistent.The update to require
pocket-icversion4.0.0is consistent across the codebase and aligns with the application's requirements. No other versions are supported, indicating this change is intentional.
cli/commands/bench-replica.ts: Version check forpocket-icis4.0.0.cli/commands/replica.ts: Version check forpocket-icis4.0.0.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the updated version check for `pocket-ic`. # Test: Search for the version check. Expect: Correct alignment with application requirements. rg --type typescript -A 5 $'pocket-ic'Length of output: 72
Script:
#!/bin/bash # Description: Verify the updated version check for `pocket-ic`. # Test: Search for the version check in TypeScript files. Expect: Correct alignment with application requirements. rg --type ts -A 5 $'pocket-ic'Length of output: 10114
39-42: Verify the modified initialization and teardown logic forPocketIcServer.Ensure that the new initialization and teardown logic is correctly implemented and manages resources efficiently without introducing memory leaks or other issues.
Also applies to: 51-53
69-69: Verify the modified method signature forsetupCanister.Ensure that the modified method signature aligns with the requirements and dependencies of the application and does not introduce any issues in canister setup.
cli/commands/publish.ts (1)
274-274: Enhanced testing configuration.The modification to the
testWithReporterfunction call to include additional parameters ('', 'interpreter', config.toolchain?.['pocket-ic'] ? 'pocket-ic' : 'dfx') is a significant improvement. It allows for more flexible and context-aware testing configurations.The code changes are approved.
Run the following script to verify the function usage:
Verification successful
Enhanced testing configuration verified.
The
testWithReporterfunction call incli/commands/publish.tsmatches the updated signature, and the function definition incli/commands/test/test.tssupports these parameters. This confirms that the changes are correctly implemented and enhance the testing configuration as intended.
cli/commands/publish.ts: The function call matches the new signature.cli/commands/test/test.ts: The function definition supports the new parameters.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `testWithReporter` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type typescript -A 5 $'testWithReporter'Length of output: 79
Script:
#!/bin/bash # Description: Verify all function calls to `testWithReporter` match the new signature. # Test: Search for the function usage in .ts files. Expect: Only occurrences of the new signature. rg 'testWithReporter' --glob '*.ts' -A 5Length of output: 1594
cli/commands/test/test.ts (2)
Line range hint
125-139: Enhanced control over testing process.The expansion of the
runAllfunction signature to includereplicaTypeandwatchparameters is a positive change. It allows the function to adapt its behavior based on the testing mode and watch settings, providing more control and flexibility.The code changes are approved.
Run the following script to verify the function usage:
Verification successful
Function calls match the updated signature.
The
runAllfunction calls in the codebase correctly match the updated signature, including thereplicaTypeandwatchparameters. No issues were found with the function usage.
- Calls to
runAllare consistent with the new signature.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `runAll` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type typescript -A 5 $'runAll'Length of output: 69
Script:
#!/bin/bash # Description: Find all TypeScript files and search for `runAll` function calls. # Locate all TypeScript files and search for `runAll` function calls. fd --extension ts --exec rg 'runAll' {}Length of output: 378
64-89: Refined testing function with enhanced configurability.The update to the
testfunction to accept aPartial<TestOptions>object and the handling of thewatchoption are significant improvements. These changes allow for more flexible and robust testing configurations, especially in managing different testing environments.The code changes are approved.
Run the following script to verify the function usage:
Verification successful
Function
testis used correctly with the new signature.The
testfunction is called with the updated signature incli/cli.ts, confirming that the changes have been correctly integrated into the codebase. No issues were found with its usage.
cli/cli.ts: Thetestfunction is called with the new signature.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `test` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type typescript -A 5 $'test'Length of output: 67
Script:
#!/bin/bash # Description: Verify all function calls to `test` match the new signature. # Test: Search for the function usage in .ts files. Expect: Only occurrences of the new signature. rg 'test' --glob '*.ts' -A 5Length of output: 24599
cli/cli.ts (3)
37-37: Enhanced state management for replica testing.The addition of the global variable
mopsReplicaTestRunningis a positive change. It enhances state management for testing scenarios, particularly for replica testing.The code changes are approved.
234-238: Enhanced flexibility and capabilities for thetestcommand.The updates to the
testcommand, including the new--replica <replica>option and the expansion of the--mode <mode>option to includereplica, significantly enhance the flexibility and capabilities of the testing command. These changes accommodate different operational contexts and extend the testing capabilities.The code changes are approved.
247-254: Improved clarity and control for thebenchcommand.The modification to the
benchcommand to remove the default value for the--replica <replica>option is a positive change. It improves clarity and control by requiring users to explicitly specify the replica for benchmarking, potentially avoiding unintended behavior or confusion.The code changes are approved.
| actor { | ||
| public func runTests() : async () { | ||
| ExperimentalCycles.add<system>(1_000_000_000_000); | ||
| var storage = await Storage.Storage(); | ||
|
|
||
| let fileId = "test"; | ||
|
|
||
| Debug.print("lalalalalalala"); | ||
|
|
||
| // upload | ||
| await suite("storage upload", func() : async () { | ||
| await test("try to finish upload before upload start", func() : async () { | ||
| let res = await storage.finishUploads([fileId]); | ||
| assert Result.isErr(res); | ||
| }); | ||
|
|
||
| await test("try to upload chunk before upload start", func() : async () { | ||
| assert Result.isErr(await storage.uploadChunk(fileId, 0, Blob.fromArray([]))); | ||
| }); | ||
|
|
||
| await test("start upload", func() : async () { | ||
| assert Result.isOk(await storage.startUpload({ | ||
| id = fileId; | ||
| path = "test/test.mo"; | ||
| chunkCount = 1; | ||
| owners = []; | ||
| })); | ||
| }); | ||
|
|
||
| await test("try to finish upload with unknown file id", func() : async () { | ||
| assert Result.isErr(await storage.finishUploads([fileId, "unknown-file-id"])); | ||
| }); | ||
|
|
||
| await test("finish upload", func() : async () { | ||
| assert Result.isOk(await storage.finishUploads([fileId])); | ||
| }); | ||
|
|
||
| await test("try to finish already finished upload", func() : async () { | ||
| assert Result.isErr(await storage.finishUploads([fileId])); | ||
| }); | ||
|
|
||
| await test("try to start upload existing file", func() : async () { | ||
| assert Result.isErr(await storage.startUpload({ | ||
| id = fileId; | ||
| path = "test/test.mo"; | ||
| chunkCount = 1; | ||
| owners = []; | ||
| })); | ||
| }); | ||
| }); | ||
|
|
||
| // download | ||
| await suite("storage download", func() : async () { | ||
| await test("get file meta", func() : async () { | ||
| let res = await storage.getFileMeta(fileId); | ||
| assert Result.isOk(res); | ||
| switch (res) { | ||
| case (#ok(fileMeta)) { | ||
| assert fileMeta.path == "test/test.mo"; | ||
| assert fileMeta.chunkCount == 1; | ||
| assert fileMeta.owners == []; | ||
| }; | ||
| case (_) {}; | ||
| }; | ||
| }); | ||
|
|
||
| await test("try to get file meta of unknown file", func() : async () { | ||
| let res = await storage.getFileMeta("123"); | ||
| assert Result.isErr(res); | ||
| }); | ||
|
|
||
| await test("upgrade storage canister", func() : async () { | ||
| storage := await (system Storage.Storage)(#upgrade(storage))(); | ||
| }); | ||
|
|
||
| await test("get file meta after upgrade", func() : async () { | ||
| let res = await storage.getFileMeta(fileId); | ||
| assert Result.isOk(res); | ||
| switch (res) { | ||
| case (#ok(fileMeta)) { | ||
| assert fileMeta.path == "test/test.mo"; | ||
| assert fileMeta.chunkCount == 1; | ||
| assert fileMeta.owners == []; | ||
| }; | ||
| case (_) {}; | ||
| }; | ||
| }); | ||
| }); | ||
| }; | ||
| }; |
There was a problem hiding this comment.
Approved: Actor and test methods.
The tests are well-structured and cover essential functionalities. Consider removing the debug print statement as it seems to be a leftover from debugging.
Remove the debug print statement:
- Debug.print("lalalalalalala");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.
| actor { | |
| public func runTests() : async () { | |
| ExperimentalCycles.add<system>(1_000_000_000_000); | |
| var storage = await Storage.Storage(); | |
| let fileId = "test"; | |
| Debug.print("lalalalalalala"); | |
| // upload | |
| await suite("storage upload", func() : async () { | |
| await test("try to finish upload before upload start", func() : async () { | |
| let res = await storage.finishUploads([fileId]); | |
| assert Result.isErr(res); | |
| }); | |
| await test("try to upload chunk before upload start", func() : async () { | |
| assert Result.isErr(await storage.uploadChunk(fileId, 0, Blob.fromArray([]))); | |
| }); | |
| await test("start upload", func() : async () { | |
| assert Result.isOk(await storage.startUpload({ | |
| id = fileId; | |
| path = "test/test.mo"; | |
| chunkCount = 1; | |
| owners = []; | |
| })); | |
| }); | |
| await test("try to finish upload with unknown file id", func() : async () { | |
| assert Result.isErr(await storage.finishUploads([fileId, "unknown-file-id"])); | |
| }); | |
| await test("finish upload", func() : async () { | |
| assert Result.isOk(await storage.finishUploads([fileId])); | |
| }); | |
| await test("try to finish already finished upload", func() : async () { | |
| assert Result.isErr(await storage.finishUploads([fileId])); | |
| }); | |
| await test("try to start upload existing file", func() : async () { | |
| assert Result.isErr(await storage.startUpload({ | |
| id = fileId; | |
| path = "test/test.mo"; | |
| chunkCount = 1; | |
| owners = []; | |
| })); | |
| }); | |
| }); | |
| // download | |
| await suite("storage download", func() : async () { | |
| await test("get file meta", func() : async () { | |
| let res = await storage.getFileMeta(fileId); | |
| assert Result.isOk(res); | |
| switch (res) { | |
| case (#ok(fileMeta)) { | |
| assert fileMeta.path == "test/test.mo"; | |
| assert fileMeta.chunkCount == 1; | |
| assert fileMeta.owners == []; | |
| }; | |
| case (_) {}; | |
| }; | |
| }); | |
| await test("try to get file meta of unknown file", func() : async () { | |
| let res = await storage.getFileMeta("123"); | |
| assert Result.isErr(res); | |
| }); | |
| await test("upgrade storage canister", func() : async () { | |
| storage := await (system Storage.Storage)(#upgrade(storage))(); | |
| }); | |
| await test("get file meta after upgrade", func() : async () { | |
| let res = await storage.getFileMeta(fileId); | |
| assert Result.isOk(res); | |
| switch (res) { | |
| case (#ok(fileMeta)) { | |
| assert fileMeta.path == "test/test.mo"; | |
| assert fileMeta.chunkCount == 1; | |
| assert fileMeta.owners == []; | |
| }; | |
| case (_) {}; | |
| }; | |
| }); | |
| }); | |
| }; | |
| }; | |
| actor { | |
| public func runTests() : async () { | |
| ExperimentalCycles.add<system>(1_000_000_000_000); | |
| var storage = await Storage.Storage(); | |
| let fileId = "test"; | |
| // upload | |
| await suite("storage upload", func() : async () { | |
| await test("try to finish upload before upload start", func() : async () { | |
| let res = await storage.finishUploads([fileId]); | |
| assert Result.isErr(res); | |
| }); | |
| await test("try to upload chunk before upload start", func() : async () { | |
| assert Result.isErr(await storage.uploadChunk(fileId, 0, Blob.fromArray([]))); | |
| }); | |
| await test("start upload", func() : async () { | |
| assert Result.isOk(await storage.startUpload({ | |
| id = fileId; | |
| path = "test/test.mo"; | |
| chunkCount = 1; | |
| owners = []; | |
| })); | |
| }); | |
| await test("try to finish upload with unknown file id", func() : async () { | |
| assert Result.isErr(await storage.finishUploads([fileId, "unknown-file-id"])); | |
| }); | |
| await test("finish upload", func() : async () { | |
| assert Result.isOk(await storage.finishUploads([fileId])); | |
| }); | |
| await test("try to finish already finished upload", func() : async () { | |
| assert Result.isErr(await storage.finishUploads([fileId])); | |
| }); | |
| await test("try to start upload existing file", func() : async () { | |
| assert Result.isErr(await storage.startUpload({ | |
| id = fileId; | |
| path = "test/test.mo"; | |
| chunkCount = 1; | |
| owners = []; | |
| })); | |
| }); | |
| }); | |
| // download | |
| await suite("storage download", func() : async () { | |
| await test("get file meta", func() : async () { | |
| let res = await storage.getFileMeta(fileId); | |
| assert Result.isOk(res); | |
| switch (res) { | |
| case (#ok(fileMeta)) { | |
| assert fileMeta.path == "test/test.mo"; | |
| assert fileMeta.chunkCount == 1; | |
| assert fileMeta.owners == []; | |
| }; | |
| case (_) {}; | |
| }; | |
| }); | |
| await test("try to get file meta of unknown file", func() : async () { | |
| let res = await storage.getFileMeta("123"); | |
| assert Result.isErr(res); | |
| }); | |
| await test("upgrade storage canister", func() : async () { | |
| storage := await (system Storage.Storage)(#upgrade(storage))(); | |
| }); | |
| await test("get file meta after upgrade", func() : async () { | |
| let res = await storage.getFileMeta(fileId); | |
| assert Result.isOk(res); | |
| switch (res) { | |
| case (#ok(fileMeta)) { | |
| assert fileMeta.path == "test/test.mo"; | |
| assert fileMeta.chunkCount == 1; | |
| assert fileMeta.owners == []; | |
| }; | |
| case (_) {}; | |
| }; | |
| }); | |
| }); | |
| }; | |
| }; |
| actor { | ||
| public func runTests() : async () { | ||
| ExperimentalCycles.add<system>(1_000_000_000_000); | ||
| var storage = await Storage.Storage(); | ||
|
|
||
| let fileId = "test"; | ||
|
|
||
| Debug.print(debug_show("lalalalalalala")); | ||
|
|
||
| // upload | ||
| await suite("storage upload", func() : async () { | ||
| await test("try to finish upload before upload start", func() : async () { | ||
| let res = await storage.finishUploads([fileId]); | ||
| assert Result.isErr(res); | ||
| }); | ||
|
|
||
| // assert false; | ||
|
|
||
| await test("try to upload chunk before upload start", func() : async () { | ||
| assert Result.isErr(await storage.uploadChunk(fileId, 0, Blob.fromArray([]))); | ||
| }); | ||
|
|
||
| await test("start upload", func() : async () { | ||
| assert Result.isOk(await storage.startUpload({ | ||
| id = fileId; | ||
| path = "test/test.mo"; | ||
| chunkCount = 1; | ||
| owners = []; | ||
| })); | ||
| }); | ||
|
|
||
| await test("try to finish upload with unknown file id", func() : async () { | ||
| assert Result.isErr(await storage.finishUploads([fileId, "unknown-file-id"])); | ||
| }); | ||
|
|
||
| await test("finish upload", func() : async () { | ||
| assert Result.isOk(await storage.finishUploads([fileId])); | ||
| }); | ||
|
|
||
| await test("try to finish already finished upload", func() : async () { | ||
| assert Result.isErr(await storage.finishUploads([fileId])); | ||
| }); | ||
|
|
||
| await test("try to start upload existing file", func() : async () { | ||
| assert Result.isErr(await storage.startUpload({ | ||
| id = fileId; | ||
| path = "test/test.mo"; | ||
| chunkCount = 1; | ||
| owners = []; | ||
| })); | ||
| }); | ||
| }); | ||
|
|
||
| // download | ||
| await suite("storage download", func() : async () { | ||
| await test("get file meta", func() : async () { | ||
| let res = await storage.getFileMeta(fileId); | ||
| assert Result.isOk(res); | ||
| switch (res) { | ||
| case (#ok(fileMeta)) { | ||
| assert fileMeta.path == "test/test.mo"; | ||
| assert fileMeta.chunkCount == 1; | ||
| assert fileMeta.owners == []; | ||
| }; | ||
| case (_) {}; | ||
| }; | ||
| }); | ||
|
|
||
| await test("try to get file meta of unknown file", func() : async () { | ||
| let res = await storage.getFileMeta("123"); | ||
| assert Result.isErr(res); | ||
| }); | ||
|
|
||
| await test("upgrade storage canister", func() : async () { | ||
| storage := await (system Storage.Storage)(#upgrade(storage))(); | ||
| }); | ||
|
|
||
| await test("get file meta after upgrade", func() : async () { | ||
| let res = await storage.getFileMeta(fileId); | ||
| assert Result.isOk(res); | ||
| switch (res) { | ||
| case (#ok(fileMeta)) { | ||
| assert fileMeta.path == "test/test.mo"; | ||
| assert fileMeta.chunkCount == 1; | ||
| assert fileMeta.owners == []; | ||
| }; | ||
| case (_) {}; | ||
| }; | ||
| }); | ||
| }); | ||
| }; | ||
| }; |
There was a problem hiding this comment.
Approved: Actor and test methods.
The tests are well-structured and cover essential functionalities. Consider removing the debug print statement as it seems to be a leftover from debugging.
Remove the debug print statement:
- Debug.print(debug_show("lalalalalalala"));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.
| actor { | |
| public func runTests() : async () { | |
| ExperimentalCycles.add<system>(1_000_000_000_000); | |
| var storage = await Storage.Storage(); | |
| let fileId = "test"; | |
| Debug.print(debug_show("lalalalalalala")); | |
| // upload | |
| await suite("storage upload", func() : async () { | |
| await test("try to finish upload before upload start", func() : async () { | |
| let res = await storage.finishUploads([fileId]); | |
| assert Result.isErr(res); | |
| }); | |
| // assert false; | |
| await test("try to upload chunk before upload start", func() : async () { | |
| assert Result.isErr(await storage.uploadChunk(fileId, 0, Blob.fromArray([]))); | |
| }); | |
| await test("start upload", func() : async () { | |
| assert Result.isOk(await storage.startUpload({ | |
| id = fileId; | |
| path = "test/test.mo"; | |
| chunkCount = 1; | |
| owners = []; | |
| })); | |
| }); | |
| await test("try to finish upload with unknown file id", func() : async () { | |
| assert Result.isErr(await storage.finishUploads([fileId, "unknown-file-id"])); | |
| }); | |
| await test("finish upload", func() : async () { | |
| assert Result.isOk(await storage.finishUploads([fileId])); | |
| }); | |
| await test("try to finish already finished upload", func() : async () { | |
| assert Result.isErr(await storage.finishUploads([fileId])); | |
| }); | |
| await test("try to start upload existing file", func() : async () { | |
| assert Result.isErr(await storage.startUpload({ | |
| id = fileId; | |
| path = "test/test.mo"; | |
| chunkCount = 1; | |
| owners = []; | |
| })); | |
| }); | |
| }); | |
| // download | |
| await suite("storage download", func() : async () { | |
| await test("get file meta", func() : async () { | |
| let res = await storage.getFileMeta(fileId); | |
| assert Result.isOk(res); | |
| switch (res) { | |
| case (#ok(fileMeta)) { | |
| assert fileMeta.path == "test/test.mo"; | |
| assert fileMeta.chunkCount == 1; | |
| assert fileMeta.owners == []; | |
| }; | |
| case (_) {}; | |
| }; | |
| }); | |
| await test("try to get file meta of unknown file", func() : async () { | |
| let res = await storage.getFileMeta("123"); | |
| assert Result.isErr(res); | |
| }); | |
| await test("upgrade storage canister", func() : async () { | |
| storage := await (system Storage.Storage)(#upgrade(storage))(); | |
| }); | |
| await test("get file meta after upgrade", func() : async () { | |
| let res = await storage.getFileMeta(fileId); | |
| assert Result.isOk(res); | |
| switch (res) { | |
| case (#ok(fileMeta)) { | |
| assert fileMeta.path == "test/test.mo"; | |
| assert fileMeta.chunkCount == 1; | |
| assert fileMeta.owners == []; | |
| }; | |
| case (_) {}; | |
| }; | |
| }); | |
| }); | |
| }; | |
| }; | |
| actor { | |
| public func runTests() : async () { | |
| ExperimentalCycles.add<system>(1_000_000_000_000); | |
| var storage = await Storage.Storage(); | |
| let fileId = "test"; | |
| // upload | |
| await suite("storage upload", func() : async () { | |
| await test("try to finish upload before upload start", func() : async () { | |
| let res = await storage.finishUploads([fileId]); | |
| assert Result.isErr(res); | |
| }); | |
| // assert false; | |
| await test("try to upload chunk before upload start", func() : async () { | |
| assert Result.isErr(await storage.uploadChunk(fileId, 0, Blob.fromArray([]))); | |
| }); | |
| await test("start upload", func() : async () { | |
| assert Result.isOk(await storage.startUpload({ | |
| id = fileId; | |
| path = "test/test.mo"; | |
| chunkCount = 1; | |
| owners = []; | |
| })); | |
| }); | |
| await test("try to finish upload with unknown file id", func() : async () { | |
| assert Result.isErr(await storage.finishUploads([fileId, "unknown-file-id"])); | |
| }); | |
| await test("finish upload", func() : async () { | |
| assert Result.isOk(await storage.finishUploads([fileId])); | |
| }); | |
| await test("try to finish already finished upload", func() : async () { | |
| assert Result.isErr(await storage.finishUploads([fileId])); | |
| }); | |
| await test("try to start upload existing file", func() : async () { | |
| assert Result.isErr(await storage.startUpload({ | |
| id = fileId; | |
| path = "test/test.mo"; | |
| chunkCount = 1; | |
| owners = []; | |
| })); | |
| }); | |
| }); | |
| // download | |
| await suite("storage download", func() : async () { | |
| await test("get file meta", func() : async () { | |
| let res = await storage.getFileMeta(fileId); | |
| assert Result.isOk(res); | |
| switch (res) { | |
| case (#ok(fileMeta)) { | |
| assert fileMeta.path == "test/test.mo"; | |
| assert fileMeta.chunkCount == 1; | |
| assert fileMeta.owners == []; | |
| }; | |
| case (_) {}; | |
| }; | |
| }); | |
| await test("try to get file meta of unknown file", func() : async () { | |
| let res = await storage.getFileMeta("123"); | |
| assert Result.isErr(res); | |
| }); | |
| await test("upgrade storage canister", func() : async () { | |
| storage := await (system Storage.Storage)(#upgrade(storage))(); | |
| }); | |
| await test("get file meta after upgrade", func() : async () { | |
| let res = await storage.getFileMeta(fileId); | |
| assert Result.isOk(res); | |
| switch (res) { | |
| case (#ok(fileMeta)) { | |
| assert fileMeta.path == "test/test.mo"; | |
| assert fileMeta.chunkCount == 1; | |
| assert fileMeta.owners == []; | |
| }; | |
| case (_) {}; | |
| }; | |
| }); | |
| }); | |
| }; | |
| }; |
| if (oldDfxJsonData.canisters) { | ||
| newDfxJsonData.canisters = Object.assign(oldDfxJsonData.canisters, newDfxJsonData.canisters); | ||
| } | ||
|
|
||
| fs.mkdirSync(this.dir, {recursive: true}); | ||
| fs.writeFileSync(dfxJson, JSON.stringify(newDfxJsonData, null, 2)); | ||
|
|
||
| execSync(`dfx deploy ${name} --mode reinstall --yes --identity anonymous`, {cwd: this.dir, stdio: this.verbose ? 'pipe' : ['pipe', 'ignore', 'pipe']}); | ||
| execSync(`dfx ledger fabricate-cycles --canister ${name} --t 100`, {cwd: this.dir, stdio: this.verbose ? 'pipe' : ['pipe', 'ignore', 'pipe']}); | ||
|
|
||
| let canisterId = execSync(`dfx canister id ${name}`, {cwd: this.dir}).toString().trim(); | ||
|
|
||
| let actor = Actor.createActor(idlFactory, { | ||
| agent: await HttpAgent.create({ | ||
| host: 'http://127.0.0.1:4945', | ||
| shouldFetchRootKey: true, | ||
| }), | ||
| canisterId, | ||
| }); | ||
|
|
||
| this.canisters[name] = { | ||
| cwd, | ||
| canisterId, | ||
| actor, | ||
| stream: new PassThrough(), | ||
| }; | ||
| } | ||
| else if (this.pocketIc) { | ||
| // let {canisterId, actor} = await this.pocketIc.setupCanister(idlFactory, wasm); | ||
| let {canisterId, actor} = await this.pocketIc.setupCanister({ | ||
| idlFactory, | ||
| wasm, | ||
| }); | ||
| await this.pocketIc.addCycles(canisterId, 1_000_000_000_000); | ||
| this.canisters[name] = { | ||
| cwd, | ||
| canisterId: canisterId.toText(), | ||
| actor, | ||
| stream: new PassThrough(), | ||
| }; | ||
| } | ||
|
|
||
| if (!this.canisters[name]) { | ||
| throw new Error(`Canister ${name} not found`); | ||
| } | ||
|
|
||
| return this.canisters[name]; | ||
| } | ||
|
|
||
| getActor(name : string) : unknown { | ||
| if (!this.canisters[name]) { | ||
| throw new Error(`Canister ${name} not found`); | ||
| } | ||
| return this.canisters[name]?.actor; | ||
| } | ||
|
|
||
| getCanister(name : string) { | ||
| return this.canisters[name]; | ||
| } | ||
|
|
||
| getCanisterId(name : string) : string { | ||
| return this.canisters[name]?.canisterId || ''; | ||
| } | ||
|
|
||
| getCanisterStream(canisterId : string) : PassThrough | null { | ||
| for (let canister of Object.values(this.canisters)) { | ||
| if (canister.canisterId === canisterId) { | ||
| return canister.stream; | ||
| } | ||
| } | ||
| return null; | ||
| } | ||
|
|
||
| dfxJson(canisterName : string, wasmPath = 'canister.wasm', didPath = 'canister.did') { | ||
| let canisters : Record<string, any> = {}; | ||
| if (canisterName) { | ||
| canisters[canisterName] = { | ||
| type: 'custom', | ||
| wasm: wasmPath, | ||
| candid: didPath, | ||
| }; | ||
| } | ||
|
|
||
| return { | ||
| version: 1, | ||
| canisters, | ||
| defaults: { | ||
| build: { | ||
| packtool: 'mops sources', | ||
| }, | ||
| }, | ||
| networks: { | ||
| local: { | ||
| type: 'ephemeral', | ||
| bind: '127.0.0.1:4945', | ||
| }, | ||
| }, | ||
| }; | ||
| } | ||
| } |
There was a problem hiding this comment.
Review of the Replica class
The Replica class is well-structured and handles different types of replicas (dfx and pocket-ic). It includes methods for starting, stopping, deploying canisters, and handling logs. The use of TypeScript enhances type safety and readability.
However, there are several areas that could be improved:
- Error Handling: The methods could benefit from more robust error handling, especially in network requests and file operations.
- Logging: The logging could be made more consistent and configurable, especially in production environments.
- Configuration Management: The handling of configuration, especially around toolchain versions and paths, could be more robust to avoid potential issues with version mismatches or configuration errors.
Overall, the class provides a solid foundation but could be refined to improve robustness and maintainability.
| export function checkConfigFile(exit = false) { | ||
| let configFile = getClosestConfigFile(); | ||
| if (!configFile) { | ||
| console.log(chalk.red('Error: ') + `Config file 'mops.toml' not found. Please run ${chalk.green('mops init')} first`); | ||
| if (exit) { | ||
| process.exit(1); | ||
| } |
There was a problem hiding this comment.
Enhanced control flow in checkConfigFile
The addition of the exit parameter to the checkConfigFile function is a positive change that enhances the function's flexibility in handling errors. This allows the function to terminate the program based on the presence of a configuration file, which is useful in scenarios where the configuration is critical for the operation of the application.
The code change is approved. However, it is recommended to ensure that this change is well-documented and covered by unit tests to prevent any unexpected behavior.
Would you like help with adding documentation or unit tests for this change?
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- cli/cli.ts (3 hunks)
- cli/commands/test/test.ts (10 hunks)
- cli/mops.ts (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- cli/commands/test/test.ts
- cli/mops.ts
Additional comments not posted (4)
cli/cli.ts (4)
34-34: Global variable declaration reviewed.The declaration of
mopsReplicaTestRunningas a global variable is noted. While this is acceptable, consider encapsulating global variables within a class or module to avoid polluting the global namespace and to enhance modularity.
237-237: Updated replica option for benchmarking reviewed.The modification to the
--replica <replica>option in the bench command, removing the default value, is a prudent change to prevent potential confusion. Consider updating the documentation or help text to clearly inform users that they must now explicitly specify the replica for benchmarking.
224-224: Enhanced test mode option reviewed.The addition of 'replica' as a choice for the
--modeoption in the test command is a positive enhancement, aligning with the PR's objectives. Ensure that the implementation of this new mode is handled correctly in the test command's action.Verification successful
Implementation of 'replica' mode verified.
The 'replica' mode is correctly integrated into the test command's action, with appropriate handling for starting and stopping the replica. The logic in
cli/commands/test/test.tsconfirms that the mode is managed as expected. No further action is required.
cli/commands/test/test.ts: Contains logic for handling the 'replica' mode.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of the 'replica' mode in the test command's action. # Test: Search for the implementation details of the 'replica' mode. Expect: Proper handling of the mode. rg --type typescript -A 5 $'test.*--mode.*replica'Length of output: 86
Script:
#!/bin/bash # Description: Verify the implementation of the 'replica' mode in the test command's action. # Test: Search for the implementation details of the 'replica' mode. Expect: Proper handling of the mode. rg --type ts -A 5 $'test.*--mode.*replica'Length of output: 43
Script:
#!/bin/bash # Description: Search for any occurrences of 'replica' in TypeScript files to verify its implementation. # Test: Search for the keyword 'replica' in the codebase. Expect: Find where and how it is being used. rg --type ts 'replica'Length of output: 5794
225-225: New replica option for testing reviewed.The addition of the
--replica <replica>option to specify the replica for testing is a valuable enhancement. Ensure that this option is correctly implemented and integrated into the test command's action.
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range, codebase verification and nitpick comments (2)
cli/commands/test/test.ts (2)
24-27: Add documentation for new imports.The new imports (
Replica,ActorMethod,PassThrough,Readable) are crucial for the functionality but lack inline documentation. It's important to document the purpose and usage of these imports to maintain code readability and ease future maintenance.Consider adding comments above each import to explain their roles, especially for less obvious ones like
ActorMethodandPassThrough.
Line range hint
130-333: Improve error handling and cleanup intestWithReporter.The function
testWithReporteris complex and handles multiple modes of operation. It lacks comprehensive error handling and cleanup, especially in asynchronous operations and file operations.Refactor the function to include better error handling and ensure that resources are cleaned up properly to prevent potential resource leaks or unhandled errors:
async function testWithReporter(reporterName: ReporterName | Reporter | undefined, filter: string, defaultMode: TestMode, replicaType: ReplicaName, watch: false): Promise<boolean> { try { // existing implementation... } catch (error) { console.error('Error during test execution:', error); return false; } finally { if (replicaStartPromise && !watch) { await replica.stop(); fs.rmSync(testTempDir, {recursive: true, force: true}); } } return reporter.done(); }This ensures that all resources are properly released and errors are handled gracefully.
| type ReplicaName = 'dfx' | 'pocket-ic'; | ||
|
|
||
| type TestOptions = { | ||
| watch ?: boolean; | ||
| reporter ?: ReporterName; | ||
| mode ?: TestMode; | ||
| watch : boolean; | ||
| reporter : ReporterName; | ||
| mode : TestMode; | ||
| replica : ReplicaName; |
There was a problem hiding this comment.
Define TestOptions type more robustly.
The TestOptions type is defined with properties that are crucial for the testing configuration. However, the type definition could be enhanced by providing default values or making some properties optional to reflect the Partial<TestOptions> usage in function signatures.
Consider refining the TestOptions type definition to include optional properties or default values where appropriate, to align with how it's used in function parameters:
type TestOptions = {
watch?: boolean;
reporter?: ReporterName;
mode?: TestMode;
replica?: ReplicaName;
};| export async function test(filter = '', options : Partial<TestOptions> = {}) { | ||
| let config = readConfig(); | ||
| let rootDir = getRootDir(); | ||
|
|
||
| if (watch) { | ||
| let replicaType = options.replica ?? (config.toolchain?.['pocket-ic'] ? 'pocket-ic' : 'dfx' as ReplicaName); | ||
| replica.type = replicaType; | ||
|
|
||
| if (options.watch) { | ||
| replica.ttl = 60 * 15; // 15 minutes | ||
|
|
||
| let sigint = false; | ||
| process.on('SIGINT', () => { | ||
| if (sigint) { | ||
| console.log('Force exit'); | ||
| process.exit(0); | ||
| } | ||
| sigint = true; | ||
|
|
||
| if (replicaStartPromise) { | ||
| console.log('Stopping replica...'); | ||
| replica.stop(true).then(() => { | ||
| process.exit(0); | ||
| }); | ||
| } | ||
| }); | ||
|
|
There was a problem hiding this comment.
Refactor the handling of SIGINT for cleaner code.
The handling of the SIGINT signal within the test function is complex and could be refactored for better readability and maintainability. The current implementation uses a flag sigint to handle repeated SIGINT signals, which complicates the flow.
Consider extracting the SIGINT handling into a separate function to simplify the test function and improve code clarity:
function setupSigintHandling(replica: Replica) {
let sigintReceived = false;
process.on('SIGINT', () => {
if (sigintReceived) {
console.log('Force exit');
process.exit(0);
}
sigintReceived = true;
console.log('Stopping replica...');
replica.stop(true).then(() => {
process.exit(0);
});
});
}Then, call this function from within the test function if options.watch is true.
| async function runAll(reporterName : ReporterName | undefined, filter = '', mode : TestMode = 'interpreter', replicaType : ReplicaName, watch = false) : Promise<boolean> { | ||
| let done = await testWithReporter(reporterName, filter, mode, replicaType, watch); |
There was a problem hiding this comment.
Ensure proper error handling in runAll.
The function runAll calls testWithReporter and returns its result. However, there is no error handling if testWithReporter throws an exception, which could lead to unhandled promise rejections.
Add try-catch blocks to handle potential exceptions from testWithReporter to ensure the function is robust against errors:
async function runAll(reporterName: ReporterName | undefined, filter: string, mode: TestMode, replicaType: ReplicaName, watch: false): Promise<boolean> {
try {
let done = await testWithReporter(reporterName, filter, mode, replicaType, watch);
return done;
} catch (error) {
console.error('Error running all tests:', error);
return false;
}
}| function pipeStdoutToMMF(stdout : Readable, mmf : MMF1) { | ||
| stdout.on('data', (data) => { | ||
| for (let line of data.toString().split('\n')) { | ||
| line = line.trim(); | ||
| if (line) { | ||
| mmf.parseLine(line); | ||
| } | ||
| }); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| function pipeStderrToMMF(stderr : Readable, mmf : MMF1, dir = '') { | ||
| stderr.on('data', (data) => { | ||
| let text : string = data.toString().trim(); | ||
| let failedLine = ''; | ||
|
|
||
| text = text.replace(/([\w+._/-]+):(\d+).(\d+)(-\d+.\d+)/g, (_m0, m1 : string, m2 : string, m3 : string) => { | ||
| // change absolute file path to relative | ||
| // change :line:col-line:col to :line:col to work in vscode | ||
| let res = `${absToRel(m1)}:${m2}:${m3}`; | ||
| let file = path.join(dir, m1); | ||
|
|
||
| // stderr | ||
| proc.stderr.on('data', (data) => { | ||
| let text : string = data.toString().trim(); | ||
| let failedLine = ''; | ||
| text = text.replace(/([\w+._/-]+):(\d+).(\d+)(-\d+.\d+)/g, (_m0, m1 : string, m2 : string, m3 : string) => { | ||
| // change absolute file path to relative | ||
| // change :line:col-line:col to :line:col to work in vscode | ||
| let res = `${absToRel(m1)}:${m2}:${m3}`; | ||
|
|
||
| if (!fs.existsSync(m1)) { | ||
| return res; | ||
| } | ||
|
|
||
| // show failed line | ||
| let content = fs.readFileSync(m1); | ||
| let lines = content.toString().split('\n') || []; | ||
| failedLine += chalk.dim('\n ...'); | ||
| let lineBefore = lines[+m2 - 2]; | ||
| if (lineBefore) { | ||
| failedLine += chalk.dim(`\n ${+m2 - 1}\t| ${lineBefore.replaceAll('\t', ' ')}`); | ||
| } | ||
| failedLine += `\n${chalk.redBright`->`} ${m2}\t| ${lines[+m2 - 1]?.replaceAll('\t', ' ')}`; | ||
| if (lines.length > +m2) { | ||
| failedLine += chalk.dim(`\n ${+m2 + 1}\t| ${lines[+m2]?.replaceAll('\t', ' ')}`); | ||
| } | ||
| failedLine += chalk.dim('\n ...'); | ||
| if (!fs.existsSync(file)) { | ||
| return res; | ||
| }); | ||
| if (failedLine) { | ||
| text += failedLine; | ||
| } | ||
| mmf.fail(text); | ||
|
|
||
| // show failed line | ||
| let content = fs.readFileSync(file); | ||
| let lines = content.toString().split('\n') || []; | ||
|
|
||
| failedLine += chalk.dim('\n ...'); | ||
|
|
||
| let lineBefore = lines[+m2 - 2]; | ||
| if (lineBefore) { | ||
| failedLine += chalk.dim(`\n ${+m2 - 1}\t| ${lineBefore.replaceAll('\t', ' ')}`); | ||
| } | ||
| failedLine += `\n${chalk.redBright`->`} ${m2}\t| ${lines[+m2 - 1]?.replaceAll('\t', ' ')}`; | ||
| if (lines.length > +m2) { | ||
| failedLine += chalk.dim(`\n ${+m2 + 1}\t| ${lines[+m2]?.replaceAll('\t', ' ')}`); | ||
| } | ||
| failedLine += chalk.dim('\n ...'); | ||
| return res; | ||
| }); | ||
| if (failedLine) { | ||
| text += failedLine; | ||
| } | ||
| mmf.fail(text); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Optimize error reporting in pipeStderrToMMF.
The function pipeStderrToMMF processes error data and attempts to provide context by reading file content synchronously within a loop. This could be inefficient and potentially block the event loop if the files are large or if there are many errors.
Consider optimizing the file reading and error processing to be asynchronous and less frequent. This could involve caching file contents or restructuring how errors are processed to minimize the impact on performance:
async function pipeStderrToMMF(stderr: Readable, mmf: MMF1, dir = '') {
stderr.on('data', async (data) => {
let text: string = data.toString().trim();
// Process errors asynchronously and optimize file reading...
});
}This change would help ensure that the application remains responsive even under high error conditions.
Summary by CodeRabbit
New Features
Replicaclass for managing and deploying canisters indfxandpocket-icenvironments.replicatesting mode in the CLI, enhancing testing flexibility.Bug Fixes
srategytostrategyin theMMF1class.Documentation
Chores
pic-icandpocket-icto newer versions.Tests