Add tests for dotrain add order lsp file #1719
Conversation
|
""" WalkthroughA new test module was introduced to the Changes
Sequence Diagram(s)sequenceDiagram
participant Test as test_problems()
participant Doc as TextDocumentItem
participant LSP as DotrainAddOrderLsp
Test->>Doc: Create TextDocumentItem with test Rainlang text
Test->>LSP: Instantiate DotrainAddOrderLsp with Doc
Test->>LSP: Call problems()
LSP-->>Test: Return list of problems (diagnostics)
Test->>Test: Assert problems match expected diagnostics
Assessment against linked issues
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (16)
🔇 Additional comments (3)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
|
@hardyjosh I didn't add any tests for hover and completion functions because they aren't used anywhere in both webapp and tauri app. I could not figure out how they are used so if you have any input on this, that would be awesome. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
crates/common/src/dotrain_add_order_lsp.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: git-clean
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, test-js-bindings)
- GitHub Check: build-tauri (ubuntu-22.04, true)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-legal)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-static)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-test)
- GitHub Check: test
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-static)
- GitHub Check: test
- GitHub Check: test
- GitHub Check: Deploy-Preview
- GitHub Check: standard-tests (ubuntu-latest, ob-rs-test, true)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-artifacts, true)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-test)
🔇 Additional comments (3)
crates/common/src/dotrain_add_order_lsp.rs (3)
123-126: Appropriate test module setup and imports.The test module is correctly set up with the
#[cfg(test)]attribute and imports the necessary components. This is a good practice for isolating test code.
128-206: Well-structured test data with appropriate test cases.The
TEXTconstant provides comprehensive test coverage with various sections (tokens, orders, scenarios, deployments) and intentionally includes different kinds of errors to test problem detection. This approach ensures thorough testing of the LSP functionality.
208-215: Clean helper function implementation.The
get_text_documenthelper function is well-implemented, making the test more readable by encapsulating the creation of the test document.
| #[tokio::test] | ||
| #[cfg(not(target_family = "wasm"))] // Keep cfg consistent with the problems function | ||
| async fn test_problems() { | ||
| let lsp = DotrainAddOrderLsp::new(get_text_document(), HashMap::new()); | ||
| let problems = lsp.problems("https://rpc.com", None, None).await; | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider using a mock or configurable RPC URL.
While the test correctly exercises the problems method, using a hardcoded URL "https://rpc.com" might be problematic if the code actually tries to connect to it.
- let problems = lsp.problems("https://rpc.com", None, None).await;
+ // Use a clearly fake URL or environment variable for testing
+ let problems = lsp.problems("http://localhost:invalid-test-url", None, None).await;📝 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.
| #[tokio::test] | |
| #[cfg(not(target_family = "wasm"))] // Keep cfg consistent with the problems function | |
| async fn test_problems() { | |
| let lsp = DotrainAddOrderLsp::new(get_text_document(), HashMap::new()); | |
| let problems = lsp.problems("https://rpc.com", None, None).await; | |
| #[tokio::test] | |
| #[cfg(not(target_family = "wasm"))] // Keep cfg consistent with the problems function | |
| async fn test_problems() { | |
| let lsp = DotrainAddOrderLsp::new(get_text_document(), HashMap::new()); | |
| // Use a clearly fake URL or environment variable for testing | |
| let problems = lsp.problems("http://localhost:invalid-test-url", None, None).await; |
| let expected_msgs = vec![ | ||
| "invalid reference to binding: raindex-subparser, only literal bindings can be referenced", | ||
| "invalid expression line", | ||
| "invalid expression line", | ||
| "invalid word pattern: ABC", | ||
| "elided binding 'fixed-io-output-token': The output token that the fixed io is for. If this doesn't match the runtime output then the fixed-io will be inverted.", | ||
| "elided binding 'fixed-io': The io ratio for the limit order.", | ||
| "undefined word: test", | ||
| "unexpected token", | ||
| ]; | ||
| let actual_msgs: Vec<String> = problems.iter().map(|p| p.msg.clone()).collect(); | ||
|
|
||
| assert_eq!(problems.len(), 8); | ||
| assert_eq!(actual_msgs, expected_msgs, "Mismatch in problem messages"); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Thorough validation of problem detection.
The test effectively validates both the number of problems and their specific messages, which is excellent for ensuring the error detection logic works correctly. Consider extending it to also check problem positions and error codes for more complete validation.
assert_eq!(problems.len(), 8);
assert_eq!(actual_msgs, expected_msgs, "Mismatch in problem messages");
+
+ // Also validate positions and error codes for complete testing
+ let expected_codes = vec![
+ /* Add expected error codes here */
+ ];
+ let actual_codes: Vec<_> = problems.iter().map(|p| p.code).collect();
+ assert_eq!(actual_codes, expected_codes, "Mismatch in error codes");📝 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.
| let expected_msgs = vec![ | |
| "invalid reference to binding: raindex-subparser, only literal bindings can be referenced", | |
| "invalid expression line", | |
| "invalid expression line", | |
| "invalid word pattern: ABC", | |
| "elided binding 'fixed-io-output-token': The output token that the fixed io is for. If this doesn't match the runtime output then the fixed-io will be inverted.", | |
| "elided binding 'fixed-io': The io ratio for the limit order.", | |
| "undefined word: test", | |
| "unexpected token", | |
| ]; | |
| let actual_msgs: Vec<String> = problems.iter().map(|p| p.msg.clone()).collect(); | |
| assert_eq!(problems.len(), 8); | |
| assert_eq!(actual_msgs, expected_msgs, "Mismatch in problem messages"); | |
| } | |
| assert_eq!(problems.len(), 8); | |
| assert_eq!(actual_msgs, expected_msgs, "Mismatch in problem messages"); | |
| // Also validate positions and error codes for complete testing | |
| let expected_codes = vec![ | |
| /* Add expected error codes here */ | |
| ]; | |
| let actual_codes: Vec<_> = problems.iter().map(|p| p.code).collect(); | |
| assert_eq!(actual_codes, expected_codes, "Mismatch in error codes"); | |
| } |
There was a problem hiding this comment.
No need to do this
There was a problem hiding this comment.
@findolor Understood, no problem! The existing validation in your test is already thorough in checking both the number and content of error messages.
Motivation
See issue: #1625
Solution
Checks
By submitting this for review, I'm confirming I've done the following:
[ ] included screenshots (if this involves a front-end change)fix #1625
Summary by CodeRabbit