feat: watch mode for patterns test#298
Conversation
Ashu999
changed the title
WalkthroughWalkthroughThis update introduces a watch mode for the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant FileSystem
participant TestRunner
User->>+CLI: run `grit patterns test --watch`
CLI->>+FileSystem: Watch for changes in .grit directory
FileSystem-->>CLI: Notify on file changes
CLI->>+TestRunner: Re-run affected pattern tests
TestRunner->>CLI: Return test results
CLI-->>User: Display test results
Assessment against linked issues
Recent review detailsConfiguration used: .coderabbit.yaml Files selected for processing (7)
Files skipped from review due to trivial changes (1)
Files skipped from review as they are similar to previous changes (6)
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}
| fn enable_watch_mode(cmd_arg: PatternsTestArgs, cmd_flags: GlobalFormatFlags) -> () { | ||
| let path = Path::new(".grit"); | ||
| // setup debouncer | ||
| let (tx, rx) = std::sync::mpsc::channel(); | ||
| // notify backend configuration | ||
| let backend_config = notify::Config::default().with_poll_interval(Duration::from_millis(10)); | ||
| // debouncer configuration | ||
| let debouncer_config = Config::default() | ||
| .with_timeout(Duration::from_millis(10)) | ||
| .with_notify_config(backend_config); | ||
| // select backend via fish operator, here PollWatcher backend | ||
| let mut debouncer = new_debouncer_opt::<_, notify::PollWatcher>(debouncer_config, tx).unwrap(); | ||
|
|
||
| debouncer | ||
| .watcher() | ||
| .watch(path, RecursiveMode::Recursive) | ||
| .unwrap(); | ||
| log::info!("\n[Watch Mode] enabled on path: {}", path.display()); | ||
|
|
||
| // event pocessing | ||
| for result in rx { | ||
| match result { | ||
| Ok(event) => { | ||
| let modified_file_path = event.get(0).unwrap().path.clone(); | ||
| log::info!("\n[Watch Mode] File modified: {:?}", modified_file_path); | ||
|
|
||
| let mut arg = cmd_arg.clone(); | ||
| let flags = cmd_flags.clone(); | ||
| arg.watch = false; //avoid creating infinite watchers | ||
| tokio::task::spawn(async move { | ||
| let affected_patterns_names = | ||
| get_affected_patterns(modified_file_path).await.unwrap(); | ||
|
|
||
| info!( | ||
| "[Watch Mode] Patterns changed/affected : {:?}\n[Watch Mode] Re-running test for changed/affected patterns...\n", | ||
| affected_patterns_names | ||
| ); | ||
|
|
||
| let _ = run_patterns_test(arg, flags, Some(affected_patterns_names)).await; | ||
| }); | ||
| } | ||
| Err(error) => { | ||
| log::error!("Error {error:?}") | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The implementation of watch mode in enable_watch_mode function is well-structured. However, consider adding error handling for the unwrap calls to prevent potential panics in production.
- let mut debouncer = new_debouncer_opt::<_, notify::PollWatcher>(debouncer_config, tx).unwrap();
+ let mut debouncer = new_debouncer_opt::<_, notify::PollWatcher>(debouncer_config, tx)
+ .expect("Failed to create debouncer");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.
| fn enable_watch_mode(cmd_arg: PatternsTestArgs, cmd_flags: GlobalFormatFlags) -> () { | |
| let path = Path::new(".grit"); | |
| // setup debouncer | |
| let (tx, rx) = std::sync::mpsc::channel(); | |
| // notify backend configuration | |
| let backend_config = notify::Config::default().with_poll_interval(Duration::from_millis(10)); | |
| // debouncer configuration | |
| let debouncer_config = Config::default() | |
| .with_timeout(Duration::from_millis(10)) | |
| .with_notify_config(backend_config); | |
| // select backend via fish operator, here PollWatcher backend | |
| let mut debouncer = new_debouncer_opt::<_, notify::PollWatcher>(debouncer_config, tx).unwrap(); | |
| debouncer | |
| .watcher() | |
| .watch(path, RecursiveMode::Recursive) | |
| .unwrap(); | |
| log::info!("\n[Watch Mode] enabled on path: {}", path.display()); | |
| // event pocessing | |
| for result in rx { | |
| match result { | |
| Ok(event) => { | |
| let modified_file_path = event.get(0).unwrap().path.clone(); | |
| log::info!("\n[Watch Mode] File modified: {:?}", modified_file_path); | |
| let mut arg = cmd_arg.clone(); | |
| let flags = cmd_flags.clone(); | |
| arg.watch = false; //avoid creating infinite watchers | |
| tokio::task::spawn(async move { | |
| let affected_patterns_names = | |
| get_affected_patterns(modified_file_path).await.unwrap(); | |
| info!( | |
| "[Watch Mode] Patterns changed/affected : {:?}\n[Watch Mode] Re-running test for changed/affected patterns...\n", | |
| affected_patterns_names | |
| ); | |
| let _ = run_patterns_test(arg, flags, Some(affected_patterns_names)).await; | |
| }); | |
| } | |
| Err(error) => { | |
| log::error!("Error {error:?}") | |
| } | |
| } | |
| } | |
| } | |
| fn enable_watch_mode(cmd_arg: PatternsTestArgs, cmd_flags: GlobalFormatFlags) -> () { | |
| let path = Path::new(".grit"); | |
| // setup debouncer | |
| let (tx, rx) = std::sync::mpsc::channel(); | |
| // notify backend configuration | |
| let backend_config = notify::Config::default().with_poll_interval(Duration::from_millis(10)); | |
| // debouncer configuration | |
| let debouncer_config = Config::default() | |
| .with_timeout(Duration::from_millis(10)) | |
| .with_notify_config(backend_config); | |
| // select backend via fish operator, here PollWatcher backend | |
| let mut debouncer = new_debouncer_opt::<_, notify::PollWatcher>(debouncer_config, tx) | |
| .expect("Failed to create debouncer"); | |
| debouncer | |
| .watcher() | |
| .watch(path, RecursiveMode::Recursive) | |
| .unwrap(); | |
| log::info!("\n[Watch Mode] enabled on path: {}", path.display()); | |
| // event pocessing | |
| for result in rx { | |
| match result { | |
| Ok(event) => { | |
| let modified_file_path = event.get(0).unwrap().path.clone(); | |
| log::info!("\n[Watch Mode] File modified: {:?}", modified_file_path); | |
| let mut arg = cmd_arg.clone(); | |
| let flags = cmd_flags.clone(); | |
| arg.watch = false; //avoid creating infinite watchers | |
| tokio::task::spawn(async move { | |
| let affected_patterns_names = | |
| get_affected_patterns(modified_file_path).await.unwrap(); | |
| info!( | |
| "[Watch Mode] Patterns changed/affected : {:?}\n[Watch Mode] Re-running test for changed/affected patterns...\n", | |
| affected_patterns_names | |
| ); | |
| let _ = run_patterns_test(arg, flags, Some(affected_patterns_names)).await; | |
| }); | |
| } | |
| Err(error) => { | |
| log::error!("Error {error:?}") | |
| } | |
| } | |
| } | |
| } |
| async fn get_affected_patterns(modified_file_path: PathBuf) -> Result<Vec<String>> { | ||
| let modified_file_path_string = modified_file_path.into_os_string().into_string().unwrap(); | ||
|
|
||
| let (patterns, _) = resolve_from_cwd(&Source::Local).await?; | ||
| let affected_patterns = collect_testable_patterns(patterns.clone()) | ||
| .into_iter() | ||
| .filter(|p| { | ||
| p.config.path.is_some() && p.config.path.as_ref().unwrap() == &modified_file_path_string | ||
| }) | ||
| .collect::<Vec<_>>(); | ||
|
|
||
| let mut affected_patterns_names = <Vec<String>>::new(); | ||
| for affected_pattern in &affected_patterns { | ||
| affected_patterns_names.push(affected_pattern.local_name.clone().unwrap()); | ||
| } | ||
| let mut affected_patterns_dependents_names = <Vec<String>>::new(); | ||
|
|
||
| let grit_files = get_grit_files_from_cwd().await?; | ||
| let current_dir = std::env::current_dir()?; | ||
| let resolver = GritModuleResolver::new(current_dir.to_str().unwrap()); | ||
|
|
||
| for p in patterns { | ||
| let body = format!("{}()", p.local_name); | ||
| let lang = PatternLanguage::get_language(&p.body); | ||
| let grit_files = grit_files.get_language_directory_or_default(lang)?; | ||
| let rich_pattern = resolver | ||
| .make_pattern(&body, Some(p.local_name.to_string())) | ||
| .unwrap(); | ||
| let src = rich_pattern.body; | ||
| let mut parser = MarzanoGritParser::new()?; | ||
| let src_tree = parser.parse_file(&src, Some(Path::new(DEFAULT_FILE_NAME)))?; | ||
| let root = src_tree.root_node(); | ||
| let is_multifile = is_multifile(&root, &grit_files, &mut parser)?; | ||
|
|
||
| let dependents = get_dependents_of_affected_patterns( | ||
| &grit_files, | ||
| &src, | ||
| &mut parser, | ||
| !is_multifile, | ||
| &affected_patterns_names, | ||
| )?; | ||
|
|
||
| for d in dependents { | ||
| if !affected_patterns_dependents_names.contains(&d) { | ||
| affected_patterns_dependents_names.push(d); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| for depen in affected_patterns_dependents_names { | ||
| if !affected_patterns_names.contains(&depen) { | ||
| affected_patterns_names.push(depen); | ||
| } | ||
| } | ||
| Ok(affected_patterns_names) | ||
| } |
There was a problem hiding this comment.
Tip
Codebase Verification
The search results indicate that there are numerous test functions across the codebase, but none specifically target the get_affected_patterns function. This suggests that there might not be direct unit tests for this function.
To ensure comprehensive testing of the get_affected_patterns function, it is crucial to add or verify the existence of tests that cover various scenarios, especially complex dependency scenarios.
- Action Required: Add unit tests for the
get_affected_patternsfunction to ensure it correctly handles complex dependency scenarios.
Analysis chain
The get_affected_patterns function effectively identifies patterns affected by file modifications. Ensure comprehensive testing to validate that it correctly handles complex dependency scenarios.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that `get_affected_patterns` correctly identifies affected patterns and their dependents.
# Test: Run unit tests for `get_affected_patterns`.
cat tests/patterns_test.rs | grep "test_get_affected_patterns"
Length of output: 117
Script:
#!/bin/bash
# Locate the definition of the `get_affected_patterns` function
ast-grep --lang rust --pattern 'async fn get_affected_patterns($_) -> $_ { $$$ }'
# Locate test functions related to `get_affected_patterns`
rg 'get_affected_patterns' --type rust
Length of output: 5187
Script:
#!/bin/bash
# Search for test functions in the `crates/cli/src/commands/patterns_test.rs` file
rg '#\[test\]' crates/cli/src/commands/patterns_test.rs -A 2
Length of output: 62
Script:
#!/bin/bash
# Search for test functions across the entire codebase
rg '#\[test\]' -A 2 --type rust
Length of output: 138229
morgante
left a comment
morgante
left a comment
There was a problem hiding this comment.
Thanks for the start, but this should be doable in a cleaner way with less code.
| pub(crate) async fn run_patterns_test( | ||
| arg: PatternsTestArgs, | ||
| flags: GlobalFormatFlags, | ||
| watch_mode_args: Option<Vec<String>>, |
There was a problem hiding this comment.
This doesn't feel like the right interface.
I think you should probably extract the core / repeated logic into a test_patterns function. Then the filtering of which patterns to run is kind of the same whether it's based on watch or exclude or something else.
There was a problem hiding this comment.
I was thinking of using arg.filter here, but it only takes single string as input and uses regex.
If it's ok with you, I can modify the behavior of filter to support multiple arguments, not use regex and filter based on exact pattern name instead. This is how I have seen most of the testing tools work, when targeting a specific test(s).
There was a problem hiding this comment.
You don't need to change the interface. No the point is you should have an inner function with a signature like test_patterns(..., targeted_patterns: Vec<String>). The outer / top functions (like arg.filter) chooses which patterns to run, then invokes the inner function to do the actual execution. The same function can also be used for the watch inner, since the core of what it does is just identifying the correct subset of patterns to run.
There was a problem hiding this comment.
I think I understood the point (from 1st comment) about keeping the filtration logic together, since they share similar purpose and variables. I can take some common filtration logic from get_marzano_pattern_test_results as well.
regarding the test execution, it happens inside get_marzano_pattern_test_results, are you suggesting to modify it into a new function test_patterns and make it an inner function?
There was a problem hiding this comment.
No it doesn't need to be an inner function. You might not need a new function actually, but instead a structure like this:
run_patterns_test(), root that handles computing available patterns, invokes the initial test, and (if configured) callsenable_watch_modewith the patterns found earlier.enable_watch_modelistens to notifications, and if files are updated, updates the patterns associated with those files. Importantly, it should not invokerun_patterns_test()or re-discover patterns - just (a) mutate the patterns based on the file change and (b) invokeget_marzano_pattern_test_resultsitself.- If necessary, you could have a
do_patterns_testfunction that abstracts some of the shared logic fromenable_watch_modeandrun_patterns_testbut it actually looks unnecessary now that I look deeper.
| let (patterns, _) = resolve_from_cwd(&Source::Local).await?; | ||
| let affected_patterns = collect_testable_patterns(patterns.clone()) |
There was a problem hiding this comment.
Rebuilding this from scratch every time is inefficient. We should instead just be looking at the existing retrieve patterns that were first found in the original top level command, then filtering those down to the ones related to the modified file.
There was a problem hiding this comment.
addressed in: #298 (comment), also code is now refactored (moved filter logic inside run_patterns_test()) to read patterns, testable only once per modification trigger.
| } | ||
| Ok(affected_patterns_names) | ||
| } | ||
|
|
There was a problem hiding this comment.
Reparsing shouldn't be necessary. If we need additional metadata as the patterns are built in the first place, collect that in the compiler.
There was a problem hiding this comment.
can you explain this a little more, how to collect the required data in in the compiler? does it happen inside filter_libs() in compiler.rs?
There was a problem hiding this comment.
The point is you shouldn't need to reparse any/all patterns to get the dependency tree. Every pattern is already parsed at least one when running the initial test, so you should build the mapping when that happens instead of running again.
… filter, compiler code
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (1)
crates/cli/src/commands/patterns_test.rs (1)
Line range hint
258-304: The implementation of therun_patterns_testfunction with the newwatch_mode_argparameter is well-structured. However, consider handling the case where thewatch_mode_argisNonemore explicitly to avoid potential confusion about the function's behavior in non-watch mode.if let Some(watch_mode_arg) = watch_mode_arg { // Existing watch mode logic here } else { // Logic for non-watch mode }
| fn enable_watch_mode(cmd_arg: PatternsTestArgs, cmd_flags: GlobalFormatFlags) { | ||
| let path = Path::new(".grit"); | ||
| let ignore_path = [".grit/.gritmodules", ".gitignore", ".grit/*.log"]; | ||
| // setup debouncer | ||
| let (tx, rx) = std::sync::mpsc::channel(); | ||
| // notify backend configuration | ||
| let backend_config = notify::Config::default().with_poll_interval(Duration::from_millis(10)); | ||
| // debouncer configuration | ||
| let debouncer_config = Config::default() | ||
| .with_timeout(Duration::from_millis(10)) | ||
| .with_notify_config(backend_config); | ||
| // select backend via fish operator, here PollWatcher backend | ||
| let mut debouncer = new_debouncer_opt::<_, notify::PollWatcher>(debouncer_config, tx).unwrap(); | ||
|
|
||
| debouncer | ||
| .watcher() | ||
| .watch(path, RecursiveMode::Recursive) | ||
| .unwrap(); | ||
| log::info!("\n[Watch Mode] enabled on path: {}", path.display()); | ||
|
|
||
| // event pocessing | ||
| for result in rx { | ||
| match result { | ||
| Ok(event) => 'event_block: { | ||
| let modified_file_path = event | ||
| .get(0) | ||
| .unwrap() | ||
| .path | ||
| .clone() | ||
| .into_os_string() | ||
| .into_string() | ||
| .unwrap(); | ||
| //temorary fix, until notify crate adds support for ignoring paths | ||
| for path in &ignore_path { | ||
| if modified_file_path.contains(path) { | ||
| break 'event_block; | ||
| } | ||
| } | ||
| log::info!("\n[Watch Mode] File modified: {:?}", modified_file_path); | ||
|
|
||
| let mut arg = cmd_arg.clone(); | ||
| let flags = cmd_flags.clone(); | ||
| arg.watch = false; //avoid creating infinite watchers | ||
| tokio::task::spawn(async move { | ||
| let _ = run_patterns_test(arg, flags, Some(modified_file_path)).await; | ||
| }); | ||
| } | ||
| Err(error) => { | ||
| log::error!("Error {error:?}") | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The enable_watch_mode function effectively sets up file watching and event handling. However, the use of .unwrap() could lead to panics in production if errors occur. Replace .unwrap() with more robust error handling.
let mut debouncer = new_debouncer_opt::<_, notify::PollWatcher>(debouncer_config, tx)
.expect("Failed to create debouncer");| fn get_affected_patterns( | ||
| modified_file_path: String, | ||
| patterns: Vec<ResolvedGritDefinition>, | ||
| libs: &PatternsDirectory, | ||
| testable_patterns: &Vec<GritPatternTestInfo>, | ||
| ) -> Result<Vec<String>> { | ||
| let affected_patterns = testable_patterns | ||
| .iter() | ||
| .filter(|p| { | ||
| p.config.path.is_some() && p.config.path.as_ref().unwrap() == &modified_file_path | ||
| }) | ||
| .collect::<Vec<_>>(); | ||
|
|
||
| let mut affected_patterns_names = <Vec<String>>::new(); | ||
| for affected_pattern in &affected_patterns { | ||
| affected_patterns_names.push(affected_pattern.local_name.clone().unwrap()); | ||
| } | ||
| let mut affected_patterns_dependents_names = <Vec<String>>::new(); | ||
|
|
||
| let current_dir = std::env::current_dir()?; | ||
| let resolver = GritModuleResolver::new(current_dir.to_str().unwrap()); | ||
|
|
||
| for p in patterns { | ||
| let body = format!("{}()", p.local_name); | ||
| let lang = PatternLanguage::get_language(&p.body); | ||
| let libs = libs.get_language_directory_or_default(lang)?; | ||
| let rich_pattern = resolver | ||
| .make_pattern(&body, Some(p.local_name.to_string())) | ||
| .unwrap(); | ||
| let src = rich_pattern.body; | ||
| let mut parser = MarzanoGritParser::new()?; | ||
| let src_tree = parser.parse_file(&src, Some(Path::new(DEFAULT_FILE_NAME)))?; | ||
| let root = src_tree.root_node(); | ||
| let is_multifile = is_multifile(&root, &libs, &mut parser)?; | ||
|
|
||
| let dependents = get_dependents_of_target_patterns( | ||
| &libs, | ||
| &src, | ||
| &mut parser, | ||
| !is_multifile, | ||
| &affected_patterns_names, | ||
| )?; | ||
|
|
||
| for d in dependents { | ||
| if !affected_patterns_dependents_names.contains(&d) { | ||
| affected_patterns_dependents_names.push(d); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| for depen in affected_patterns_dependents_names { | ||
| if !affected_patterns_names.contains(&depen) { | ||
| affected_patterns_names.push(depen); | ||
| } | ||
| } | ||
| Ok(affected_patterns_names) |
There was a problem hiding this comment.
Tip
Codebase Verification
The search results indicate that there are numerous test functions across the codebase, but none of them seem to be directly related to the get_affected_patterns function. Additionally, the only occurrence of get_affected_patterns is in the crates/cli/src/commands/patterns_test.rs file, which does not contain any test functions.
Given this information, it appears that the get_affected_patterns function is not currently covered by unit tests. This is a significant oversight, especially considering the function's importance in determining affected patterns based on file changes.
To address this, I recommend adding unit tests for the get_affected_patterns function to ensure it handles complex dependency scenarios correctly.
- Add unit tests for the
get_affected_patternsfunction incrates/cli/src/commands/patterns_test.rs.
Analysis chain
The get_affected_patterns function is crucial for determining which patterns are affected by file changes. Ensure that this function is covered by unit tests, especially to handle complex dependency scenarios.
The previous attempts to locate unit tests for the get_affected_patterns function did not yield any results. To ensure thorough verification, we should search for all test functions in the crates/cli/src/commands/patterns_test.rs file and manually inspect if any of them test the get_affected_patterns function.
The previous attempts to locate test functions in crates/cli/src/commands/patterns_test.rs did not yield any results. This suggests that there might not be any test functions in that file, or they might be named differently.
To ensure thorough verification, we should search for all test functions across the entire codebase and check if any of them test the get_affected_patterns function.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that `get_affected_patterns` correctly identifies affected patterns and their dependents.
# Test: Run unit tests for `get_affected_patterns`.
cat tests/patterns_test.rs | grep "test_get_affected_patterns"
Length of output: 117
Script:
#!/bin/bash
# Locate the definition of the `get_affected_patterns` function
ast-grep --lang rust --pattern 'fn get_affected_patterns($_, $_, $_, $_) -> $_ { $$$ }'
# Search for test functions that might be testing `get_affected_patterns`
rg 'get_affected_patterns' --type rust
Length of output: 5042
Script:
#!/bin/bash
# Search for test functions in `crates/cli/src/commands/patterns_test.rs` and check if they call `get_affected_patterns`
rg -A 5 'fn test_' crates/cli/src/commands/patterns_test.rs | grep -B 5 'get_affected_patterns'
Length of output: 95
Script:
#!/bin/bash
# List all test functions in `crates/cli/src/commands/patterns_test.rs`
rg 'fn test_' crates/cli/src/commands/patterns_test.rs
Length of output: 56
Script:
#!/bin/bash
# List all test functions across the entire codebase
rg 'fn test_' --type rust
# Search for calls to `get_affected_patterns` across the entire codebase
rg 'get_affected_patterns' --type rust
Length of output: 11294
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Outside diff range and nitpick comments (2)
crates/cli/src/commands/patterns_test.rs (2)
6-6: Consider adding a comment explaining the purpose ofDEFAULT_FILE_NAMEfor better code clarity.
444-485: The functionget_dependents_of_target_patternsis well-structured but consider adding comments to explain the logic, especially how dependencies are resolved.
| pub fn get_dependents_of_target_patterns_by_traversal_from_src( | ||
| libs: &BTreeMap<String, String>, | ||
| src: &str, | ||
| parser: &mut MarzanoGritParser, | ||
| will_autowrap: bool, | ||
| target_patterns: &Vec<String>, | ||
| ) -> Result<Vec<String>> { | ||
| let mut dependents = <Vec<String>>::new(); | ||
| let node_like = "nodeLike"; | ||
| let predicate_call = "predicateCall"; | ||
|
|
||
| let tree = parser.parse_file(src, Some(Path::new(DEFAULT_FILE_NAME)))?; | ||
|
|
||
| let DefsToFilenames { | ||
| patterns: pattern_file, | ||
| predicates: predicate_file, | ||
| functions: function_file, | ||
| foreign_functions: foreign_file, | ||
| } = defs_to_filenames(libs, parser, tree.root_node())?; | ||
|
|
||
| let mut traversed_stack = <Vec<String>>::new(); | ||
|
|
||
| // gross but necessary due to running these patterns before and after each file | ||
| let mut stack: Vec<Tree> = if will_autowrap { | ||
| let before_each_file = "before_each_file()"; | ||
| let before_tree = | ||
| parser.parse_file(before_each_file, Some(Path::new(DEFAULT_FILE_NAME)))?; | ||
| let after_each_file = "after_each_file()"; | ||
| let after_tree = parser.parse_file(after_each_file, Some(Path::new(DEFAULT_FILE_NAME)))?; | ||
|
|
||
| vec![tree, before_tree, after_tree] | ||
| } else { | ||
| vec![tree] | ||
| }; | ||
| while let Some(tree) = stack.pop() { | ||
| let root = tree.root_node(); | ||
| let cursor = root.walk(); | ||
|
|
||
| for n in traverse(cursor, Order::Pre).filter(|n| { | ||
| n.node.is_named() && (n.node.kind() == node_like || n.node.kind() == predicate_call) | ||
| }) { | ||
| let name = n | ||
| .child_by_field_name("name") | ||
| .ok_or_else(|| anyhow!("missing name of nodeLike"))?; | ||
| let name = name.text()?; | ||
| let name = name.trim().to_string(); | ||
|
|
||
| if target_patterns.contains(&name) { | ||
| while let Some(e) = traversed_stack.pop() { | ||
| dependents.push(e); | ||
| } | ||
| } | ||
| if n.node.kind() == node_like { | ||
| if let Some(tree) = find_child_tree_definition( | ||
| &pattern_file, | ||
| parser, | ||
| libs, | ||
| &mut traversed_stack, | ||
| &name, | ||
| )? { | ||
| stack.push(tree); | ||
| } | ||
| if let Some(tree) = find_child_tree_definition( | ||
| &function_file, | ||
| parser, | ||
| libs, | ||
| &mut traversed_stack, | ||
| &name, | ||
| )? { | ||
| stack.push(tree); | ||
| } | ||
| if let Some(tree) = find_child_tree_definition( | ||
| &foreign_file, | ||
| parser, | ||
| libs, | ||
| &mut traversed_stack, | ||
| &name, | ||
| )? { | ||
| stack.push(tree); | ||
| } | ||
| } else if n.node.kind() == predicate_call { | ||
| if let Some(tree) = find_child_tree_definition( | ||
| &predicate_file, | ||
| parser, | ||
| libs, | ||
| &mut traversed_stack, | ||
| &name, | ||
| )? { | ||
| stack.push(tree); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| Ok(dependents) | ||
| } |
There was a problem hiding this comment.
Consider optimizing the traversal logic to avoid redundant checks.
The current implementation of get_dependents_of_target_patterns_by_traversal_from_src seems to perform redundant checks and might benefit from optimization. Specifically, the traversal logic could be streamlined to avoid unnecessary stack operations and checks. Consider using a more efficient data structure or algorithm to handle the traversal and dependency resolution.
| async fn get_modified_and_deleted_patterns( | ||
| modified_path: &String, | ||
| testable_patterns: &Vec<GritPatternTestInfo>, | ||
| ) -> Result<(Vec<GritPatternTestInfo>, Vec<GritPatternTestInfo>)> { | ||
| let path = Path::new(modified_path); | ||
| let file_patterns = collect_from_file(path, &None).await?; | ||
| let modified_patterns = get_grit_pattern_test_info(file_patterns); | ||
|
|
||
| let mut modified_pattern_names = <Vec<String>>::new(); | ||
| for pattern in &modified_patterns { | ||
| modified_pattern_names.push(pattern.local_name.clone().unwrap()); | ||
| } | ||
| //modified_patterns = patterns which are updated/edited or newly created. | ||
| //deleted_patterns = patterns which are deleted. Only remaining dependents of deleted_patterns should gets tested. | ||
| let mut deleted_patterns = <Vec<GritPatternTestInfo>>::new(); | ||
| for pattern in testable_patterns { | ||
| if pattern.config.path.as_ref().unwrap() == modified_path | ||
| && !modified_pattern_names.contains(pattern.local_name.as_ref().unwrap()) | ||
| { | ||
| deleted_patterns.push(pattern.clone()); | ||
| } | ||
| } | ||
|
|
||
| Ok((modified_patterns, deleted_patterns)) |
There was a problem hiding this comment.
Ensure that the function get_modified_and_deleted_patterns handles cases where pattern.config.path might be None, which could lead to unwrapping a None value.
if let Some(pattern_path) = pattern.config.path.as_ref() {
if pattern_path == modified_path && !modified_pattern_names.contains(pattern.local_name.as_ref().unwrap()) {
deleted_patterns.push(pattern.clone());
}
} else {
log::warn!("Pattern config path is missing for pattern: {:?}", pattern.local_name);
}| pub fn get_patterns_from_yaml( | ||
| file: &RichFile, | ||
| source_module: &ModuleRepo, | ||
| source_module: &Option<ModuleRepo>, |
There was a problem hiding this comment.
Tip
Codebase Verification
The functions fetch_grit_module and get_grit_module_dir in fetcher.rs do not handle the None case for Option<ModuleRepo>. They need to be updated to manage the optional nature of ModuleRepo.
fetch_grit_moduleat lines 327 and 374 incrates/gritmodule/src/fetcher.rsget_grit_module_dirat lines 317 and 364 incrates/gritmodule/src/fetcher.rs
Analysis chain
The modification to accept an Option<ModuleRepo> in get_patterns_from_yaml enhances flexibility in handling module repositories, which is crucial for robust pattern resolution.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all functions handling `ModuleRepo` correctly manage the `None` case.
# Test: Search for functions using `ModuleRepo`. Expect: Proper handling of `None`.
rg --type rust $'fn .*\\(.*ModuleRepo.*\\)'
Length of output: 923
Script:
#!/bin/bash
# Extract the implementations of functions using `ModuleRepo` to verify handling of `Option<ModuleRepo>`
# Extract functions from config.rs
ast-grep --lang rust --pattern $'fn url(&self, local_repo: &ModuleRepo, local_path: &Path) -> String { $$$ }' crates/gritmodule/src/config.rs
# Extract functions from fetcher.rs
ast-grep --lang rust --pattern $'fn fetch_grit_module(&self, repo: &ModuleRepo) -> Result<String> { $$$ }' crates/gritmodule/src/fetcher.rs
ast-grep --lang rust --pattern $'fn clone_repo(&self, repo: &ModuleRepo, target_dir: &PathBuf) -> Result<&PathBuf> { $$$ }' crates/gritmodule/src/fetcher.rs
ast-grep --lang rust --pattern $'fn get_grit_module_dir(&self, repo: &ModuleRepo) -> PathBuf { $$$ }' crates/gritmodule/src/fetcher.rs
Length of output: 1875
| &PatternsTestArgs { | ||
| update: false, | ||
| verbose: false, | ||
| watch: false, |
There was a problem hiding this comment.
Tip
Codebase Verification
The watch field in PatternsTestArgs is defined but not utilized in any meaningful way across the codebase. It is only set to false in the initialization within plumbing.rs and not referenced elsewhere.
- patterns_test.rs: The
PatternsTestArgsstruct is used in function parameters, but thewatchfield is not referenced. - patterns.rs: The
PatternsTestArgsstruct is defined, but thewatchfield is not utilized. - plumbing.rs: The
watchfield is set tofalsebut not used in any logic.
This indicates that the watch field is not integrated into the functionality as intended.
- crates/cli/src/commands/patterns_test.rs: Lines 46, 264, 302
- crates/cli/src/commands/patterns.rs: Lines 32, 40
- crates/cli/src/commands/plumbing.rs: Lines 27, 266
Analysis chain
The addition of the watch field in PatternsTestArgs is well-implemented and aligns with the PR objectives. Ensure that this new field is correctly utilized across all relevant function calls.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the `watch` field is utilized correctly in function calls.
# Test: Search for the function usage. Expect: Correct usage of the new field.
rg --type rust $'PatternsTestArgs {.*watch.*}'
Length of output: 176
Script:
#!/bin/bash
# Description: Verify that the `watch` field is utilized correctly in function calls.
# Test: Search for the function usage. Expect: Correct usage of the new field.
rg --type rust 'PatternsTestArgs \{[^}]*watch[^}]*\}'
Length of output: 55
Script:
#!/bin/bash
# Description: Search for all instances of `PatternsTestArgs` in the codebase.
# Test: Search for all instances of `PatternsTestArgs`. Expect: Identify if the `watch` field is being utilized or omitted.
rg --type rust 'PatternsTestArgs'
Length of output: 598
Script:
#!/bin/bash
# Description: Extract and display lines where `PatternsTestArgs` is used to verify if the `watch` field is being utilized.
# Extract lines from `patterns_test.rs`
rg --type rust --line-number 'PatternsTestArgs' crates/cli/src/commands/patterns_test.rs
# Extract lines from `patterns.rs`
rg --type rust --line-number 'PatternsTestArgs' crates/cli/src/commands/patterns.rs
# Extract lines from `plumbing.rs`
rg --type rust --line-number 'PatternsTestArgs' crates/cli/src/commands/plumbing.rs
Length of output: 540
Script:
#!/bin/bash
# Description: Extract and display surrounding lines where `PatternsTestArgs` is used to verify if the `watch` field is being utilized.
# Extract surrounding lines from `patterns_test.rs`
rg --type rust --context 5 --line-number 'PatternsTestArgs' crates/cli/src/commands/patterns_test.rs
# Extract surrounding lines from `patterns.rs`
rg --type rust --context 5 --line-number 'PatternsTestArgs' crates/cli/src/commands/patterns.rs
# Extract surrounding lines from `plumbing.rs`
rg --type rust --context 5 --line-number 'PatternsTestArgs' crates/cli/src/commands/plumbing.rs
Length of output: 3336
crates/gritmodule/src/testing.rs
Outdated
| mut patterns: Vec<ModuleGritPattern>, | ||
| ) -> Vec<GritPatternTestInfo> { | ||
| let testable_patterns: Vec<GritPatternTestInfo> = patterns | ||
| .iter_mut() |
There was a problem hiding this comment.
Use into_iter and avoid needing to clone.
| let path = Path::new(modified_path); | ||
| let file_patterns = collect_from_file(path, &None).await?; | ||
| let modified_patterns = get_grit_pattern_test_info(file_patterns); | ||
|
|
There was a problem hiding this comment.
Use &String instead of &str for consistency with modified_pattern_names.
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (1)
crates/core/src/pattern_compiler/compiler.rs (1)
Line range hint
686-703: Enhance error handling insrc_to_problem_libs.The function
src_to_problem_libscould improve its error handling by explicitly checking the results ofTargetLanguage::from_treeandPatternBuilder::startbefore proceeding. This would prevent runtime panics in cases where these functions fail.
| pub fn get_dependents_of_target_patterns_by_traversal_from_src( | ||
| libs: &BTreeMap<String, String>, | ||
| src: &str, | ||
| parser: &mut MarzanoGritParser, | ||
| will_autowrap: bool, | ||
| target_patterns: &[String], | ||
| ) -> Result<Vec<String>> { | ||
| let mut dependents = <Vec<String>>::new(); | ||
| let node_like = "nodeLike"; | ||
| let predicate_call = "predicateCall"; | ||
|
|
||
| let tree = parser.parse_file(src, Some(Path::new(DEFAULT_FILE_NAME)))?; | ||
|
|
||
| let DefsToFilenames { | ||
| patterns: pattern_file, | ||
| predicates: predicate_file, | ||
| functions: function_file, | ||
| foreign_functions: foreign_file, | ||
| } = defs_to_filenames(libs, parser, tree.root_node())?; | ||
|
|
||
| let mut traversed_stack = <Vec<String>>::new(); | ||
|
|
||
| // gross but necessary due to running these patterns before and after each file | ||
| let mut stack: Vec<Tree> = if will_autowrap { | ||
| let before_each_file = "before_each_file()"; | ||
| let before_tree = | ||
| parser.parse_file(before_each_file, Some(Path::new(DEFAULT_FILE_NAME)))?; | ||
| let after_each_file = "after_each_file()"; | ||
| let after_tree = parser.parse_file(after_each_file, Some(Path::new(DEFAULT_FILE_NAME)))?; | ||
|
|
||
| vec![tree, before_tree, after_tree] | ||
| } else { | ||
| vec![tree] | ||
| }; | ||
| while let Some(tree) = stack.pop() { | ||
| let root = tree.root_node(); | ||
| let cursor = root.walk(); | ||
|
|
||
| for n in traverse(cursor, Order::Pre).filter(|n| { | ||
| n.node.is_named() && (n.node.kind() == node_like || n.node.kind() == predicate_call) | ||
| }) { | ||
| let name = n | ||
| .child_by_field_name("name") | ||
| .ok_or_else(|| anyhow!("missing name of nodeLike"))?; | ||
| let name = name.text()?; | ||
| let name = name.trim().to_string(); | ||
|
|
||
| if target_patterns.contains(&name) { | ||
| while let Some(e) = traversed_stack.pop() { | ||
| dependents.push(e); | ||
| } | ||
| } | ||
| if n.node.kind() == node_like { | ||
| if let Some(tree) = find_child_tree_definition( | ||
| &pattern_file, | ||
| parser, | ||
| libs, | ||
| &mut traversed_stack, | ||
| &name, | ||
| )? { | ||
| stack.push(tree); | ||
| } | ||
| if let Some(tree) = find_child_tree_definition( | ||
| &function_file, | ||
| parser, | ||
| libs, | ||
| &mut traversed_stack, | ||
| &name, | ||
| )? { | ||
| stack.push(tree); | ||
| } | ||
| if let Some(tree) = find_child_tree_definition( | ||
| &foreign_file, | ||
| parser, | ||
| libs, | ||
| &mut traversed_stack, | ||
| &name, | ||
| )? { | ||
| stack.push(tree); | ||
| } | ||
| } else if n.node.kind() == predicate_call { | ||
| if let Some(tree) = find_child_tree_definition( | ||
| &predicate_file, | ||
| parser, | ||
| libs, | ||
| &mut traversed_stack, | ||
| &name, | ||
| )? { | ||
| stack.push(tree); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| Ok(dependents) | ||
| } |
There was a problem hiding this comment.
Optimize the traversal logic to avoid redundant checks.
The current implementation of get_dependents_of_target_patterns_by_traversal_from_src seems to perform redundant checks and might benefit from optimization. Specifically, the traversal logic could be streamlined to avoid unnecessary stack operations and checks. Consider using a more efficient data structure or algorithm to handle the traversal and dependency resolution.
| fn find_child_tree_definition( | ||
| files: &BTreeMap<String, String>, | ||
| parser: &mut MarzanoGritParser, | ||
| libs: &BTreeMap<String, String>, | ||
| traversed_stack: &mut Vec<String>, | ||
| name: &str, | ||
| ) -> Result<Option<Tree>> { | ||
| if let Some(file_name) = files.get(name) { | ||
| if !traversed_stack.contains(&name.to_string()) { | ||
| if let Some(file_body) = libs.get(file_name) { | ||
| traversed_stack.push(name.to_owned()); | ||
| let tree = parser.parse_file(file_body, Some(Path::new(file_name)))?; | ||
| return Ok(Some(tree)); | ||
| } | ||
| } | ||
| }; | ||
| Ok(None) | ||
| } |
There was a problem hiding this comment.
Refine error handling and simplify logic in find_child_tree_definition.
Consider handling potential errors from parser.parse_file more gracefully. Additionally, the logic to check for cycles could be optimized by using a set instead of a vector for traversed_stack, which would offer O(1) complexity for the containment check.
| debouncer | ||
| .watcher() | ||
| .watch(path, RecursiveMode::Recursive) | ||
| .unwrap(); |
There was a problem hiding this comment.
| .unwrap(); | |
| ? |
| if let Some(tree) = find_child_tree_definition( | ||
| &pattern_file, | ||
| parser, | ||
| libs, | ||
| &mut traversed_stack, | ||
| &name, | ||
| )? { | ||
| stack.push(tree); | ||
| } | ||
| if let Some(tree) = find_child_tree_definition( | ||
| &function_file, | ||
| parser, | ||
| libs, | ||
| &mut traversed_stack, | ||
| &name, | ||
| )? { | ||
| stack.push(tree); | ||
| } | ||
| if let Some(tree) = find_child_tree_definition( | ||
| &foreign_file, | ||
| parser, | ||
| libs, | ||
| &mut traversed_stack, | ||
| &name, | ||
| )? { | ||
| stack.push(tree); | ||
| } | ||
| } else if n.node.kind() == predicate_call { | ||
| if let Some(tree) = find_child_tree_definition( | ||
| &predicate_file, | ||
| parser, | ||
| libs, | ||
| &mut traversed_stack, | ||
| &name, | ||
| )? { | ||
| stack.push(tree); | ||
| } |
There was a problem hiding this comment.
Needing to repeat like this is a code smell. Instead of having a separate lookup for each kind we should be able to collapse into a main map before hand then just traverse that.
| let mut traversed_stack = <Vec<String>>::new(); | ||
|
|
||
| // gross but necessary due to running these patterns before and after each file | ||
| let mut stack: Vec<Tree> = if will_autowrap { |
There was a problem hiding this comment.
Just leave this logic out - we don't need to worry about autowrap / before patterns for --watch.
| let modified_file_path = event.first().unwrap().path.clone(); | ||
|
|
||
| if !modified_file_path.is_file() { | ||
| break 'event_block; | ||
| } | ||
| let modified_file_path = modified_file_path.into_os_string().into_string().unwrap(); | ||
|
|
There was a problem hiding this comment.
Handle potential errors gracefully when unwrapping.
Avoid using .unwrap() directly on potentially None values to prevent panics.
let modified_file_path = event.first().map(|e| e.path.clone()).unwrap_or_else(|| {
log::warn!("Event does not contain a valid path.");
return;
});
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range and nitpick comments (1)
crates/cli/src/commands/plumbing.rs (1)
Line range hint
49-49: Ensure proper error handling and context in async functions.The function
run_plumbingperforms various operations that could fail, such as reading inputs and parsing JSON. It's crucial to ensure that all potential errors are handled gracefully and that sufficient context is provided for each error to aid debugging.- let input: PlumbingApplyInput = serde_json::from_str::<PlumbingApplyInput>(&buffer).map_err(|e| { - anyhow!( - "Failed to parse input JSON: {}. Ensure that input matches schema \ - {{ pattern_body: string; pattern_libs: {{ [string]: string }}; paths: string[]; }}", - e - ) - })?; + let input: PlumbingApplyInput = serde_json::from_str::<PlumbingApplyInput>(&buffer).with_context(|| format!("Failed to parse input JSON: {}", &buffer))?;
crates/gritmodule/src/resolver.rs
Outdated
| if let Some(module) = module { | ||
| let repo_root = find_repo_root_from(repo_path).await?; | ||
| get_patterns_from_yaml(&config, module, &repo_root)? | ||
| get_patterns_from_yaml(&config, &Some(module.to_owned()), &repo_root)? |
There was a problem hiding this comment.
Ensure proper error handling for get_patterns_from_yaml to prevent potential panics.
As previously commented, there's an unwrapping of the result from get_patterns_from_yaml which can lead to panics if an error occurs. It's safer to handle this potential error explicitly to avoid runtime issues.
let yaml_patterns = match get_patterns_from_yaml(&config, &Some(module.to_owned()), &repo_root) {
Ok(patterns) => patterns,
Err(e) => return Err(e.into()),
};Tip
Codebase Verification
Review the error handling for get_patterns_from_yaml across multiple locations.
-
Inconsistent error handling found:
crates/gritmodule/src/yaml.rs:let patterns = get_patterns_from_yaml(&grit_yaml, &repo, &None).unwrap();
The use of
.unwrap()can cause the application to panic if an error occurs. Consider using proper error handling mechanisms likematch,unwrap_or_else, or the?operator to ensure the application handles errors gracefully.
Please review and update the error handling as necessary.
Analysis chain
Review the handling of get_patterns_from_yaml results across multiple locations.
It's crucial to ensure consistent and robust error handling throughout the codebase. The repeated emphasis on handling errors from get_patterns_from_yaml is to ensure that the application does not crash unexpectedly.
Also applies to: 589-589
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the error handling consistency for `get_patterns_from_yaml`.
# Test: Search for all usages of `get_patterns_from_yaml` and ensure they use proper error handling.
rg --type rust $'get_patterns_from_yaml'
Length of output: 722
| #[test] | ||
| fn watch_mode_of_patterns_test() -> Result<()> { | ||
| let (tx, rx) = mpsc::channel(); | ||
| let cur_dir = std::env::var("CARGO_MANIFEST_DIR").unwrap(); | ||
| let root_dir = Path::new(&cur_dir) | ||
| .parent() | ||
| .unwrap() | ||
| .parent() | ||
| .unwrap() | ||
| .to_owned(); | ||
| let test_yaml_path = root_dir.join(".grit").join("test").join("test.yaml"); | ||
|
|
||
| let _cmd_handle = thread::spawn(move || { | ||
| let mut cmd = Command::cargo_bin("marzano") | ||
| .unwrap() | ||
| .args(&["patterns", "test", "--watch"]) | ||
| .current_dir(&root_dir) | ||
| .stdout(Stdio::piped()) | ||
| .stderr(Stdio::piped()) | ||
| .spawn() | ||
| .expect("Failed to start command"); | ||
|
|
||
| let stdout = BufReader::new(cmd.stdout.take().unwrap()); | ||
| let stderr = BufReader::new(cmd.stderr.take().unwrap()); | ||
| for line in stdout.lines().chain(stderr.lines()) { | ||
| if let Ok(line) = line { | ||
| tx.send(line).unwrap(); | ||
| } | ||
| } | ||
| }); | ||
| thread::sleep(Duration::from_secs(1)); | ||
|
|
||
| let _modifier_handle = thread::spawn(move || { | ||
| let content = fs::read_to_string(&test_yaml_path).unwrap(); | ||
| fs::write(&test_yaml_path, content).unwrap(); | ||
| }); | ||
| thread::sleep(Duration::from_secs(1)); | ||
|
|
||
| let mut output = Vec::new(); | ||
| while let Ok(line) = rx.try_recv() { | ||
| output.push(line); | ||
| } | ||
| let expected_output = vec![ | ||
| "[Watch Mode] enabled on path: .grit", | ||
| "[Watch Mode] File modified: \".grit/test/test.yaml\"", | ||
| "[Watch Mode] Pattern to test: [\"our_cargo_use_long_dependency\", \"cargo_use_long_dependency\", \"no_treesitter_in_grit_crates\", \"no_println_in_lsp\", \"no_println_in_core\"]", | ||
| "Found 5 testable patterns.", | ||
| ]; | ||
| for expected_line in expected_output { | ||
| assert!( | ||
| output.iter().any(|line| line.contains(expected_line)), | ||
| "Expected output not found: {}", | ||
| expected_line | ||
| ); | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Ensure robustness in the watch mode test implementation.
The implementation of the watch mode test is comprehensive, but it relies heavily on unwrapping, which could lead to panics. Consider adding error handling to improve the robustness of the test.
- let cur_dir = std::env::var("CARGO_MANIFEST_DIR").unwrap();
+ let cur_dir = std::env::var("CARGO_MANIFEST_DIR").expect("CARGO_MANIFEST_DIR not set");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.
| #[test] | |
| fn watch_mode_of_patterns_test() -> Result<()> { | |
| let (tx, rx) = mpsc::channel(); | |
| let cur_dir = std::env::var("CARGO_MANIFEST_DIR").unwrap(); | |
| let root_dir = Path::new(&cur_dir) | |
| .parent() | |
| .unwrap() | |
| .parent() | |
| .unwrap() | |
| .to_owned(); | |
| let test_yaml_path = root_dir.join(".grit").join("test").join("test.yaml"); | |
| let _cmd_handle = thread::spawn(move || { | |
| let mut cmd = Command::cargo_bin("marzano") | |
| .unwrap() | |
| .args(&["patterns", "test", "--watch"]) | |
| .current_dir(&root_dir) | |
| .stdout(Stdio::piped()) | |
| .stderr(Stdio::piped()) | |
| .spawn() | |
| .expect("Failed to start command"); | |
| let stdout = BufReader::new(cmd.stdout.take().unwrap()); | |
| let stderr = BufReader::new(cmd.stderr.take().unwrap()); | |
| for line in stdout.lines().chain(stderr.lines()) { | |
| if let Ok(line) = line { | |
| tx.send(line).unwrap(); | |
| } | |
| } | |
| }); | |
| thread::sleep(Duration::from_secs(1)); | |
| let _modifier_handle = thread::spawn(move || { | |
| let content = fs::read_to_string(&test_yaml_path).unwrap(); | |
| fs::write(&test_yaml_path, content).unwrap(); | |
| }); | |
| thread::sleep(Duration::from_secs(1)); | |
| let mut output = Vec::new(); | |
| while let Ok(line) = rx.try_recv() { | |
| output.push(line); | |
| } | |
| let expected_output = vec![ | |
| "[Watch Mode] enabled on path: .grit", | |
| "[Watch Mode] File modified: \".grit/test/test.yaml\"", | |
| "[Watch Mode] Pattern to test: [\"our_cargo_use_long_dependency\", \"cargo_use_long_dependency\", \"no_treesitter_in_grit_crates\", \"no_println_in_lsp\", \"no_println_in_core\"]", | |
| "Found 5 testable patterns.", | |
| ]; | |
| for expected_line in expected_output { | |
| assert!( | |
| output.iter().any(|line| line.contains(expected_line)), | |
| "Expected output not found: {}", | |
| expected_line | |
| ); | |
| } | |
| Ok(()) | |
| } | |
| #[test] | |
| fn watch_mode_of_patterns_test() -> Result<()> { | |
| let (tx, rx) = mpsc::channel(); | |
| let cur_dir = std::env::var("CARGO_MANIFEST_DIR").expect("CARGO_MANIFEST_DIR not set"); | |
| let root_dir = Path::new(&cur_dir) | |
| .parent() | |
| .unwrap() | |
| .parent() | |
| .unwrap() | |
| .to_owned(); | |
| let test_yaml_path = root_dir.join(".grit").join("test").join("test.yaml"); | |
| let _cmd_handle = thread::spawn(move || { | |
| let mut cmd = Command::cargo_bin("marzano") | |
| .unwrap() | |
| .args(&["patterns", "test", "--watch"]) | |
| .current_dir(&root_dir) | |
| .stdout(Stdio::piped()) | |
| .stderr(Stdio::piped()) | |
| .spawn() | |
| .expect("Failed to start command"); | |
| let stdout = BufReader::new(cmd.stdout.take().unwrap()); | |
| let stderr = BufReader::new(cmd.stderr.take().unwrap()); | |
| for line in stdout.lines().chain(stderr.lines()) { | |
| if let Ok(line) = line { | |
| tx.send(line).unwrap(); | |
| } | |
| } | |
| }); | |
| thread::sleep(Duration::from_secs(1)); | |
| let _modifier_handle = thread::spawn(move || { | |
| let content = fs::read_to_string(&test_yaml_path).unwrap(); | |
| fs::write(&test_yaml_path, content).unwrap(); | |
| }); | |
| thread::sleep(Duration::from_secs(1)); | |
| let mut output = Vec::new(); | |
| while let Ok(line) = rx.try_recv() { | |
| output.push(line); | |
| } | |
| let expected_output = vec![ | |
| "[Watch Mode] enabled on path: .grit", | |
| "[Watch Mode] File modified: \".grit/test/test.yaml\"", | |
| "[Watch Mode] Pattern to test: [\"our_cargo_use_long_dependency\", \"cargo_use_long_dependency\", \"no_treesitter_in_grit_crates\", \"no_println_in_lsp\", \"no_println_in_core\"]", | |
| "Found 5 testable patterns.", | |
| ]; | |
| for expected_line in expected_output { | |
| assert!( | |
| output.iter().any(|line| line.contains(expected_line)), | |
| "Expected output not found: {}", | |
| expected_line | |
| ); | |
| } | |
| Ok(()) | |
| } |
| pub fn pattern_config_to_model( | ||
| pattern: GritDefinitionConfig, | ||
| source: &ModuleRepo, | ||
| source: &Option<ModuleRepo>, |
There was a problem hiding this comment.
Refactor the function signature for clarity and simplicity.
The pattern_config_to_model function's signature is complex. Consider simplifying it or adding detailed documentation to clarify its purpose and usage.
- pub fn pattern_config_to_model(pattern: GritDefinitionConfig, source: &Option<ModuleRepo>) -> Result<ModuleGritPattern> {
+ pub fn pattern_config_to_model(pattern: GritDefinitionConfig, source: Option<&ModuleRepo>) -> Result<ModuleGritPattern> {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.
| source: &Option<ModuleRepo>, | |
| source: Option<&ModuleRepo>, |
.grit/test/test.yaml
Outdated
| @@ -0,0 +1,82 @@ | |||
| # this file is used for integration testing of `grit patterns test --watch` | |||
There was a problem hiding this comment.
Please don't invent a new way to do integration tests. We already have fixtures and existing conventions. Look at them and follow that.
.grit/grit.yaml
Outdated
| "language-submodules", | ||
| "language-metavariables" | ||
| } | ||
| - name: cargo_use_long_dependency |
There was a problem hiding this comment.
Consider renaming the pattern name our_cargo_use_long_dependency.
The current pattern name might be confusing. Consider renaming it to something more descriptive like long_dependency_in_cargo_toml.
- - name: our_cargo_use_long_dependency
+ - name: long_dependency_in_cargo_tomlCommittable 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.
| - name: cargo_use_long_dependency | |
| - name: long_dependency_in_cargo_toml |
| #[test] | ||
| fn patterns_test_watch_mode_case_patterns_changed() -> Result<()> { | ||
| let (tx, rx) = mpsc::channel(); | ||
|
|
||
| let (temp_dir, temp_grit_dir) = get_fixture(".grit", false)?; | ||
| let test_yaml_path = temp_grit_dir.join("grit.yaml"); | ||
| let temp_dir_path = temp_dir.path().to_owned(); | ||
|
|
||
| let _cmd_handle = thread::spawn(move || { | ||
| let mut cmd = Command::cargo_bin(BIN_NAME) | ||
| .unwrap() | ||
| .args(&["patterns", "test", "--watch"]) | ||
| .current_dir(&temp_dir_path) | ||
| .stdout(Stdio::piped()) | ||
| .stderr(Stdio::piped()) | ||
| .spawn() | ||
| .expect("Failed to start command"); | ||
|
|
||
| let stdout = BufReader::new(cmd.stdout.take().unwrap()); | ||
| let stderr = BufReader::new(cmd.stderr.take().unwrap()); | ||
| for line in stdout.lines().chain(stderr.lines()) { | ||
| if let Ok(line) = line { | ||
| tx.send(line).unwrap(); | ||
| } | ||
| } | ||
| }); | ||
| thread::sleep(Duration::from_secs(1)); | ||
|
|
||
| let content = fs::read_to_string(&test_yaml_path).expect("Unable to read the file"); | ||
| fs::write(&test_yaml_path, content).unwrap(); | ||
| thread::sleep(Duration::from_secs(1)); | ||
|
|
||
| let mut output = Vec::new(); | ||
| while let Ok(line) = rx.try_recv() { | ||
| output.push(line); | ||
| } | ||
| let expected_output = vec![ | ||
| "[Watch Mode] Enabled on path: .grit", | ||
| "[Watch Mode] File modified: \".grit/grit.yaml\"", | ||
| "[Watch Mode] Pattern(s) to test: [\"our_cargo_use_long_dependency\", \"cargo_use_long_dependency\", \"no_treesitter_in_grit_crates\", \"no_println_in_lsp\", \"no_println_in_core\"]", | ||
| "Found 5 testable patterns.", | ||
| ]; | ||
| for expected_line in expected_output { | ||
| assert!( | ||
| output.iter().any(|line| line.contains(expected_line)), | ||
| "Expected output not found: {}", | ||
| expected_line | ||
| ); | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Ensure robustness in the watch mode test implementation.
The implementation of the watch mode test is comprehensive, but it relies heavily on unwrapping, which could lead to panics. Consider adding error handling to improve the robustness of the test.
- let cur_dir = std::env::var("CARGO_MANIFEST_DIR").unwrap();
+ let cur_dir = std::env::var("CARGO_MANIFEST_DIR").expect("CARGO_MANIFEST_DIR not set");Committable suggestion was skipped due to low confidence.
| #[test] | ||
| fn patterns_test_watch_mode_case_no_pattern_to_test() -> Result<()> { | ||
| let (tx, rx) = mpsc::channel(); | ||
|
|
||
| let (temp_dir, temp_grit_dir) = get_fixture(".grit", false)?; | ||
| let test_yaml_path = temp_grit_dir.join("grit.yaml"); | ||
| let temp_dir_path = temp_dir.path().to_owned(); | ||
|
|
||
| let _cmd_handle = thread::spawn(move || { | ||
| let mut cmd = Command::cargo_bin(BIN_NAME) | ||
| .unwrap() | ||
| .args(&["patterns", "test", "--watch"]) | ||
| .current_dir(&temp_dir_path) | ||
| .stdout(Stdio::piped()) | ||
| .stderr(Stdio::piped()) | ||
| .spawn() | ||
| .expect("Failed to start command"); | ||
|
|
||
| let stdout = BufReader::new(cmd.stdout.take().unwrap()); | ||
| let stderr = BufReader::new(cmd.stderr.take().unwrap()); | ||
| for line in stdout.lines().chain(stderr.lines()) { | ||
| if let Ok(line) = line { | ||
| tx.send(line).unwrap(); | ||
| } | ||
| } | ||
| }); | ||
| thread::sleep(Duration::from_secs(1)); | ||
|
|
||
| fs::write(&test_yaml_path, "").unwrap(); | ||
| thread::sleep(Duration::from_secs(1)); | ||
|
|
||
| let mut output = Vec::new(); | ||
| while let Ok(line) = rx.try_recv() { | ||
| output.push(line); | ||
| } | ||
|
|
||
| let expected_output = vec![ | ||
| "[Watch Mode] Enabled on path: .grit", | ||
| "[Watch Mode] File modified: \".grit/grit.yaml\"", | ||
| "[Watch Mode] Pattern(s) to test: []", | ||
| ]; | ||
| for expected_line in expected_output { | ||
| assert!( | ||
| output.iter().any(|line| line.contains(expected_line)), | ||
| "Expected output not found: {}", | ||
| expected_line | ||
| ); | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Ensure robustness in the watch mode test implementation.
The implementation of the watch mode test is comprehensive, but it relies heavily on unwrapping, which could lead to panics. Consider adding error handling to improve the robustness of the test.
- let cur_dir = std::env::var("CARGO_MANIFEST_DIR").unwrap();
+ let cur_dir = std::env::var("CARGO_MANIFEST_DIR").expect("CARGO_MANIFEST_DIR not set");Committable suggestion was skipped due to low confidence.
morgante
left a comment
morgante
left a comment
There was a problem hiding this comment.
Try to be more disciplined with clones.
| .with_timeout(Duration::from_millis(10)) | ||
| .with_notify_config(backend_config); | ||
| // select backend via fish operator, here PollWatcher backend | ||
| let mut debouncer = new_debouncer_opt::<_, notify::PollWatcher>(debouncer_config, tx).unwrap(); |
| let modified_file_path = event.first().unwrap().path.clone(); | ||
|
|
||
| if !modified_file_path.is_file() { | ||
| continue; | ||
| } | ||
| let modified_file_path = modified_file_path.into_os_string().into_string().unwrap(); |
There was a problem hiding this comment.
Likely cloning twice unnecessarily.
There was a problem hiding this comment.
this seems to be the best available way to do it: https://stackoverflow.com/a/42579588
There was a problem hiding this comment.
Right, but the clone() on line 327 shouldn't be needed.
There was a problem hiding this comment.
yes, it can be changed to:
let modified_file_path = &event.first().unwrap().path;
if !modified_file_path.is_file() {
continue;
}
let modified_file_path = modified_file_path
.clone()
.into_os_string()
.into_string()
.unwrap();which is slightly better because of if condition in the middle. but clone() can't be avoided as into_os_string() requires owned PathBuf.
| fn map_pattern_to_test_info(pattern: ResolvedGritDefinition) -> GritPatternTestInfo { | ||
| let samples = pattern.config.samples; | ||
| GritPatternTestInfo { | ||
| body: pattern.body.clone(), | ||
| body: pattern.body, | ||
| config: GritPatternTestConfig { | ||
| path: Some(pattern.config.path.clone()), | ||
| path: Some(pattern.config.path), | ||
| samples, | ||
| }, | ||
| local_name: Some(pattern.local_name.clone()), | ||
| local_name: Some(pattern.local_name), |
There was a problem hiding this comment.
Tip
Codebase Verification
Ensure pattern.config.samples is always initialized.
The samples field in GritDefinitionConfig is of type Option<Vec<GritPatternSample>>, which means it can be None. To avoid potential runtime errors, ensure that pattern.config.samples is always initialized within the map_pattern_to_test_info function.
crates/gritmodule/src/testing.rs, lines 17-25
Analysis chain
Verify initialization of pattern.config.samples.
Ensure that pattern.config.samples is always initialized to avoid potential runtime errors.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that `pattern.config.samples` is always initialized.
# Test: Search for the initialization of `pattern.config.samples`. Expect: Initialization in all cases.
rg --type rust 'config.samples'
Length of output: 1272
Script:
#!/bin/bash
# Description: Verify the initialization of `config.samples` within `ResolvedGritDefinition` or related structures.
# Search for the definition and initialization of `ResolvedGritDefinition` and check how `config.samples` is handled.
rg --type rust 'struct ResolvedGritDefinition' -A 10
rg --type rust 'struct GritPatternConfig' -A 10
Length of output: 789
Script:
#!/bin/bash
# Description: Verify the initialization of `config.samples` within `GritDefinitionConfig`.
# Search for the definition and initialization of `GritDefinitionConfig` and check how `samples` is handled.
rg --type rust 'struct GritDefinitionConfig' -A 10
Length of output: 758
Script:
#!/bin/bash
# Description: Verify the initialization of `samples` within `GritDefinitionConfig` instances.
# Search for instances where `GritDefinitionConfig` is instantiated and check how the `samples` field is handled.
rg --type rust 'GritDefinitionConfig' -A 5 -B 5
Length of output: 8916
| Ok(None) | ||
| } | ||
|
|
||
| pub fn get_dependents_of_target_patterns_by_traversal_from_src( |
There was a problem hiding this comment.
Use Vec::new() instead of <Vec<String>>::new() for consistency.
For better readability and consistency, use Vec::new() instead of <Vec<String>>::new().
- let mut dependents = <Vec<String>>::new();
+ let mut dependents = Vec::new();Committable suggestion was skipped due to low confidence.
| traversed_stack: &mut Vec<String>, | ||
| name: &str, | ||
| ) -> Result<Option<Tree>> { | ||
| if !traversed_stack.contains(&name.to_string()) { |
There was a problem hiding this comment.
Optimize the traversal logic to avoid redundant checks.
The current implementation seems to perform redundant checks and might benefit from optimization. Specifically, the traversal logic could be streamlined to avoid unnecessary stack operations and checks.
- if !traversed_stack.contains(&name.to_string()) {
- if let Some(file_body) = libs.get(file_name) {
- traversed_stack.push(name.to_owned());
- let tree = parser.parse_file(file_body, Some(Path::new(file_name)))?;
- return Ok(Some(tree));
- }
- }
+ if traversed_stack.insert(name.to_string()) {
+ if let Some(file_body) = libs.get(file_name) {
+ let tree = parser.parse_file(file_body, Some(Path::new(file_name)))?;
+ return Ok(Some(tree));
+ }
+ }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.
| if !traversed_stack.contains(&name.to_string()) { | |
| if traversed_stack.insert(name.to_string()) { | |
| if let Some(file_body) = libs.get(file_name) { | |
| let tree = parser.parse_file(file_body, Some(Path::new(file_name)))?; | |
| return Ok(Some(tree)); | |
| } | |
| } |
| if pattern.config.path.as_ref().unwrap() == modified_path | ||
| && !modified_pattern_names.contains(&pattern.local_name.as_ref().unwrap()) | ||
| { | ||
| deleted_patterns.push(pattern.clone()); |
There was a problem hiding this comment.
Handle potential None values in pattern configuration paths.
Ensure that the function get_modified_and_deleted_patterns gracefully handles cases where pattern.config.path might be None.
- if pattern.config.path.as_ref().unwrap() == modified_path
+ if let Some(pattern_path) = pattern.config.path.as_ref() {
+ if pattern_path == modified_path && !modified_pattern_names.contains(&pattern.local_name.as_ref().unwrap()) {
+ deleted_patterns.push(pattern.clone());
+ }
+ } else {
+ log::warn!("Pattern config path is missing for pattern: {:?}", pattern.local_name);
+ }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.
| if pattern.config.path.as_ref().unwrap() == modified_path | |
| && !modified_pattern_names.contains(&pattern.local_name.as_ref().unwrap()) | |
| { | |
| deleted_patterns.push(pattern.clone()); | |
| if let Some(pattern_path) = pattern.config.path.as_ref() { | |
| if pattern_path == modified_path && !modified_pattern_names.contains(&pattern.local_name.as_ref().unwrap()) { | |
| deleted_patterns.push(pattern.clone()); | |
| } | |
| } else { | |
| log::warn!("Pattern config path is missing for pattern: {:?}", pattern.local_name); | |
| } |
| let temp_dir_path = temp_dir.path().to_owned(); | ||
|
|
||
| let _cmd_handle = thread::spawn(move || { | ||
| let mut cmd = Command::cargo_bin(BIN_NAME) |
There was a problem hiding this comment.
Use get_test_cmd() instead and avoid hard-coding the BIN_name.
There was a problem hiding this comment.
I did try to use get_test_cmd(), but was not able to use .stdout() with it.
was getting below error:
no method named stdout found for mutable reference &mut assert_cmd::Command in the current scope
method not found in `&mut Command``
There was a problem hiding this comment.
created get_test_process_cmd() to use process::Command, which supports .stdout()
There was a problem hiding this comment.
We have many other tests that inspect stdout: https://github.com/getgrit/gritql/blob/f850d3aaaefeefedad2446a230728636632b1484/crates/cli_bin/tests/help.rs#L23
I don't understand why you need a new interface.
morgante
left a comment
morgante
left a comment
There was a problem hiding this comment.
Once you resolve merge conflicts and tests pass, this is good to go.
|
@Ashu999 Tests and linters are failing. Please make sure everything passes. |
2 participants
Fixes: #51
/claim #51
Summary by CodeRabbit
New Features
grit patterns test --watch.Enhancements
Bug Fixes
Testing
Documentation
.gitignoreto ignore specific log files and grit modules.Greptile Summary
This is an auto-generated summary
compiler.rs