feat: watch mode for patterns test #298
Conversation
Ashu999
changed the title
WalkthroughWalkthroughThe recent changes introduce a watch mode for the Changes
Assessment against linked issues
Tip New Features and ImprovementsReview SettingsIntroduced new personality profiles for code reviews. Users can now select between "Chill" and "Assertive" review tones to tailor feedback styles according to their preferences. The "Assertive" profile posts more comments and nitpicks the code more aggressively, while the "Chill" profile is more relaxed and posts fewer comments. AST-based InstructionsCodeRabbit offers customizing reviews based on the Abstract Syntax Tree (AST) pattern matching. Read more about AST-based instructions in the documentation. Community-driven AST-based RulesWe are kicking off a community-driven initiative to create and share AST-based rules. Users can now contribute their AST-based rules to detect security vulnerabilities, code smells, and anti-patterns. Please see the ast-grep-essentials repository for more information. New Static Analysis ToolsWe are continually expanding our support for static analysis tools. We have added support for Tone SettingsUsers can now customize CodeRabbit to review code in the style of their favorite characters or personalities. Here are some of our favorite examples:
Revamped Settings PageWe have redesigned the settings page for a more intuitive layout, enabling users to find and adjust settings quickly. This change was long overdue; it not only improves the user experience but also allows our development team to add more settings in the future with ease. Going forward, the changes to Miscellaneous
Recent Review DetailsConfiguration used: .coderabbit.yaml Files selected for processing (2)
Additional comments not posted (1)
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
| Ok(()) | ||
| } | ||
|
|
||
| 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?
| } | ||
|
|
||
| fn enable_watch_mode(cmd_arg: PatternsTestArgs, cmd_flags: GlobalFormatFlags) -> () { | ||
| let path = Path::new(".grit"); |
There was a problem hiding this comment.
We don't want to watch all of .grit (including .gritmodules). Also this should probably not be created new, but instead should be passed in based on the original patterns retrieved.
There was a problem hiding this comment.
notify crate currently does not support ignoring the directories inside tracked directory(notify-rs/notify#598). I have added a temporary fix to avoid re-running the tests in case any of these files are modified.
There was a problem hiding this comment.
should be passed in based on the original patterns retrieved
let's say a new pattern is added to a file, we will not know that unless we re-read the patterns from file. right?
| 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?
… 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
Fixes: #51
/claim #51
WIP: tests are left
Summary by CodeRabbit
New Features
Enhancements
notifyandnotify-debouncer-minito enhance functionality.remote_workflowsandworkflow_serverfor better maintenance.Improvements
Clonetrait to various structs for improved data handling.Bug Fixes