-
Notifications
You must be signed in to change notification settings - Fork 3
feat: add include pattern matching for test file detection #22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,12 +7,28 @@ use crate::types::{ | |
| TrueAffectedConfig, | ||
| }; | ||
| use crate::utils; | ||
| use regex::Regex; | ||
| use rustc_hash::{FxHashMap, FxHashSet}; | ||
| use std::collections::HashMap; | ||
| use std::path::{Path, PathBuf}; | ||
| use std::sync::Arc; | ||
| use tracing::debug; | ||
|
|
||
| /// Default include pattern: matches test/spec files (.spec.ts, .test.tsx, etc.) | ||
| const DEFAULT_INCLUDE_PATTERN: &str = r"\.(spec|test)\.(ts|js)x?$"; | ||
|
|
||
| fn is_ignored_path(file_path: &Path, ignored_paths: &[String]) -> bool { | ||
| if ignored_paths.is_empty() { | ||
| return false; | ||
| } | ||
|
|
||
| let file_str = file_path.to_string_lossy(); | ||
| ignored_paths | ||
| .iter() | ||
| .filter(|ignored| !ignored.trim().is_empty()) | ||
| .any(|ignored| file_str.contains(ignored)) | ||
| } | ||
|
|
||
| /// Mutable state for tracking affected symbols during analysis | ||
| struct AffectedState<'a> { | ||
| affected_packages: &'a mut FxHashSet<String>, | ||
|
|
@@ -69,6 +85,45 @@ fn find_affected_internal( | |
| let mut affected_packages = FxHashSet::default(); | ||
| let mut project_causes: FxHashMap<String, Vec<AffectCause>> = FxHashMap::default(); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor: Step numbering is misleading This is labeled "Step 4b" but there is no "Step 4a" — the preceding block is "Step 4: Track affected packages". Consider renumbering to make the flow clearer (e.g., make this its own step and renumber downstream). |
||
|
|
||
| // Step 4b: Process included file patterns (e.g., test files) | ||
| // Files matching include patterns directly mark their owning project as affected, | ||
| // matching traf's changedIncludedFilesPackages behavior. | ||
| let include_patterns = if config.include.is_empty() { | ||
| vec![DEFAULT_INCLUDE_PATTERN.to_string()] | ||
|
Comment on lines
+90
to
+92
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fragile "disable default" API Using a single empty string as a sentinel to disable defaults is error-prone and non-obvious. A caller passing Consider using
This makes the intent explicit at the type level and avoids accidental opt-out. |
||
| } else if config.include.len() == 1 && config.include[0].trim().is_empty() { | ||
| vec![] | ||
| } else { | ||
| config.include.clone() | ||
| }; | ||
|
|
||
| let compiled_patterns: Vec<Regex> = include_patterns | ||
| .iter() | ||
| .filter_map(|p| match Regex::new(p) { | ||
| Ok(re) => Some(re), | ||
| Err(e) => { | ||
| debug!("Invalid include pattern '{}': {}", p, e); | ||
| None | ||
|
Comment on lines
+96
to
+105
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor perf: Regex compiled on every call
Not a blocker, but worth considering. |
||
| } | ||
| }) | ||
| .collect(); | ||
|
|
||
| for changed_file in &changed_files { | ||
| if is_ignored_path(&changed_file.file_path, &config.ignored_paths) { | ||
| continue; | ||
| } | ||
| let file_str = changed_file.file_path.to_string_lossy(); | ||
| if compiled_patterns.iter().any(|re| re.is_match(&file_str)) { | ||
| if let Some(pkg) = utils::get_package_name_by_path(&changed_file.file_path, &config.projects) | ||
| { | ||
| debug!( | ||
| "Include pattern matched {:?}, adding package '{}'", | ||
| changed_file.file_path, pkg | ||
| ); | ||
| affected_packages.insert(pkg); | ||
|
Comment on lines
+108
to
+122
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No When a test file matches an include pattern, the project is added to Consider either:
This is important for the observability contract — users running |
||
| } | ||
| } | ||
| } | ||
|
|
||
| // Step 5: Partition changed files into source and non-source | ||
| let (source_files, asset_files): (Vec<&ChangedFile>, Vec<&ChangedFile>) = changed_files | ||
| .iter() | ||
|
|
@@ -680,4 +735,26 @@ mod tests { | |
| assert!(affected.contains("lib1")); | ||
| assert!(affected.contains("app")); // Should be added as implicit dependent | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_default_include_pattern_matches_test_files() { | ||
| let re = Regex::new(DEFAULT_INCLUDE_PATTERN).expect("Default pattern should compile"); | ||
|
|
||
| // Should match spec/test files | ||
| assert!(re.is_match("proj1/utils.spec.ts")); | ||
| assert!(re.is_match("proj1/utils.test.ts")); | ||
| assert!(re.is_match("proj1/component.spec.tsx")); | ||
| assert!(re.is_match("proj1/component.test.tsx")); | ||
| assert!(re.is_match("proj1/helper.spec.js")); | ||
| assert!(re.is_match("proj1/helper.test.jsx")); | ||
| assert!(re.is_match("tests/validation/roof.step.test.ts")); | ||
|
|
||
| // Should NOT match regular source files | ||
| assert!(!re.is_match("proj1/utils.ts")); | ||
| assert!(!re.is_match("proj1/component.tsx")); | ||
| assert!(!re.is_match("proj1/index.js")); | ||
| assert!(!re.is_match("proj1/styles.css")); | ||
| assert!(!re.is_match("proj1/data.json")); | ||
| assert!(!re.is_match("proj1/test-utils.ts")); // "test" in name but not .test.ts | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1479,3 +1479,113 @@ export function anotherFn() { | |
| "proj2 should be affected via asset → constant → export chain" | ||
| ); | ||
| } | ||
|
|
||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing edge-case tests Good coverage for the happy paths! A few edge cases worth adding:
|
||
| /// Helper to get affected projects with custom include patterns | ||
| fn get_affected_with_include(include: Vec<String>) -> Vec<String> { | ||
| let config = TrueAffectedConfig { | ||
| cwd: fixture_path(), | ||
| base: "main".to_string(), | ||
| root_ts_config: Some(PathBuf::from("tsconfig.json")), | ||
| projects: vec![ | ||
| Project { | ||
| name: "proj1".to_string(), | ||
| source_root: PathBuf::from("proj1"), | ||
| ts_config: Some(PathBuf::from("proj1/tsconfig.json")), | ||
| implicit_dependencies: vec![], | ||
| targets: vec![], | ||
| }, | ||
| Project { | ||
| name: "proj2".to_string(), | ||
| source_root: PathBuf::from("proj2"), | ||
| ts_config: Some(PathBuf::from("proj2/tsconfig.json")), | ||
| implicit_dependencies: vec![], | ||
| targets: vec![], | ||
| }, | ||
| Project { | ||
| name: "proj3".to_string(), | ||
| source_root: PathBuf::from("proj3"), | ||
| ts_config: Some(PathBuf::from("proj3/tsconfig.json")), | ||
| implicit_dependencies: vec!["proj1".to_string()], | ||
| targets: vec![], | ||
| }, | ||
| ], | ||
| include, | ||
| ignored_paths: vec![], | ||
| }; | ||
|
|
||
| let profiler = Arc::new(Profiler::new(false)); | ||
|
|
||
| find_affected(config, profiler) | ||
| .expect("Failed to find affected projects") | ||
| .affected_projects | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_included_test_file_marks_project_affected() { | ||
| let branch = TestBranch::new("test-include-spec-file"); | ||
|
|
||
| // Create a .spec.ts file in proj1 (test file matching default include pattern) | ||
| branch.make_change( | ||
| "proj1/utils.spec.ts", | ||
| r#"import { proj1 } from './index'; | ||
|
|
||
| describe('proj1', () => { | ||
| it('should work', () => { | ||
| expect(proj1()).toBe('proj1'); | ||
| }); | ||
| }); | ||
| "#, | ||
| ); | ||
|
|
||
| // Now modify only the test file | ||
| branch.make_change( | ||
| "proj1/utils.spec.ts", | ||
| r#"import { proj1 } from './index'; | ||
|
|
||
| describe('proj1', () => { | ||
| it('should work correctly', () => { | ||
| expect(proj1()).toBe('proj1-modified'); | ||
| }); | ||
| }); | ||
| "#, | ||
| ); | ||
|
|
||
| // Use default include (empty vec triggers default test file pattern) | ||
| let affected = get_affected_with_include(vec![]); | ||
|
|
||
| // proj1 should be affected because the .spec.ts file matches the default include pattern | ||
| assert!( | ||
| affected.contains(&"proj1".to_string()), | ||
| "proj1 should be affected when a .spec.ts file changes (default include pattern). Got: {:?}", | ||
| affected | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_included_file_with_custom_pattern() { | ||
| let branch = TestBranch::new("test-include-custom"); | ||
|
|
||
| // Create a .stories.ts file in proj2 | ||
| branch.make_change( | ||
| "proj2/button.stories.ts", | ||
| r#"export const Primary = { args: { label: 'Click' } }; | ||
| "#, | ||
| ); | ||
|
|
||
| // Modify the stories file | ||
| branch.make_change( | ||
| "proj2/button.stories.ts", | ||
| r#"export const Primary = { args: { label: 'Click Me' } }; | ||
| "#, | ||
| ); | ||
|
|
||
| // Use a custom include pattern that matches .stories.ts files | ||
| let affected = get_affected_with_include(vec![r"\.stories\.(ts|js)x?$".to_string()]); | ||
|
|
||
| // proj2 should be affected because the .stories.ts file matches the custom pattern | ||
| assert!( | ||
| affected.contains(&"proj2".to_string()), | ||
| "proj2 should be affected when a .stories.ts file changes (custom include pattern). Got: {:?}", | ||
| affected | ||
| ); | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is_ignored_pathonly used for include patternsThis helper is introduced here but
ignored_pathsis still#[allow(dead_code)]on theTrueAffectedConfigstruct. This creates an inconsistency — are ignored paths now active or not?Should this also be applied to source/asset file processing in Steps 5/5a for full parity with traf? If scoping to include-pattern matching only is intentional, it'd be worth adding a comment explaining why.