feat: add include pattern matching for test file detection#22
feat: add include pattern matching for test file detection#22EladBezalel wants to merge 2 commits intomainfrom
Conversation
📦 Preview Release AvailableA preview release has been published for commit acd8ab4. Installationnpm install https://github.com/frontops-dev/domino/releases/download/pr-22-acd8ab4/front-ops-domino-0.3.7.tgzRunning the previewnpx https://github.com/frontops-dev/domino/releases/download/pr-22-acd8ab4/front-ops-domino-0.3.7.tgz affectedDetails |
Implement traf's `changedIncludedFilesPackages` mechanism in domino. Changed files matching configurable regex patterns now directly mark their owning project as affected, regardless of whether the file is parsed by the semantic analyzer. Default pattern matches test/spec files: `\.(spec|test)\.(ts|js)x?$` This ensures that when only test files change for a project, the project is still correctly detected as affected -- matching traf's behavior. Co-authored-by: Cursor <[email protected]>
- Include pattern matching now skips files matching ignored_paths - Passing a single empty string in include disables default patterns - Documented opt-out mechanism in TrueAffectedConfig Co-authored-by: Cursor <[email protected]>
16b5f91 to
acd8ab4
Compare
shaharkazaz
left a comment
There was a problem hiding this comment.
Good feature — this closes an important parity gap with traf. The core logic is sound and tests cover the main paths well.
Key items to address before merge:
- Add
AffectCausetracking for include-pattern matches — most important. Without this,--reportmode silently omits the reason a project was marked affected. - Reconsider the empty-string sentinel for disabling defaults —
Option<Vec<String>>would be safer and more idiomatic. - Rebase onto current
main— PR currently has merge conflicts.
Nice-to-haves:
- Clarify
is_ignored_pathscope (only includes? or also source/asset files?) - Cache the default regex pattern
- Add edge-case tests (disable sentinel, invalid regex, no owning project)
- Fix step numbering
| // matching traf's changedIncludedFilesPackages behavior. | ||
| let include_patterns = if config.include.is_empty() { | ||
| vec![DEFAULT_INCLUDE_PATTERN.to_string()] |
There was a problem hiding this comment.
Fragile "disable default" API
Using a single empty string as a sentinel to disable defaults is error-prone and non-obvious. A caller passing vec![""] by accident (e.g., splitting an empty CLI arg) would silently disable test file detection.
Consider using Option<Vec<String>> instead:
None→ use defaults (test file patterns)Some(vec![])→ explicitly disable all include patternsSome(vec!["pattern"])→ custom patterns
This makes the intent explicit at the type level and avoids accidental opt-out.
| 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() |
There was a problem hiding this comment.
is_ignored_path only used for include patterns
This helper is introduced here but ignored_paths is still #[allow(dead_code)] on the TrueAffectedConfig struct. 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.
| .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); |
There was a problem hiding this comment.
No AffectCause recorded for include-pattern matches
When a test file matches an include pattern, the project is added to affected_packages but no AffectCause entry is pushed to project_causes. This means --report mode won't explain why a project was marked affected via include patterns.
Consider either:
- Adding a new
AffectCause::IncludePatternMatch { file, pattern }variant, or - At minimum, recording a
DirectChangewith the matched file
This is important for the observability contract — users running domino affected --report will see the project listed with no explanation for why it's affected.
| @@ -69,6 +85,45 @@ fn find_affected_internal( | |||
| let mut affected_packages = FxHashSet::default(); | |||
| let mut project_causes: FxHashMap<String, Vec<AffectCause>> = FxHashMap::default(); | |||
There was a problem hiding this comment.
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).
| 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 |
There was a problem hiding this comment.
Minor perf: Regex compiled on every call
Regex::new(DEFAULT_INCLUDE_PATTERN) is called every time find_affected_internal runs. Since domino's selling point is performance, consider using std::sync::LazyLock (Rust 1.80+) or lazy_static! to compile the default pattern once.
Not a blocker, but worth considering.
| "proj2 should be affected via asset → constant → export chain" | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
Missing edge-case tests
Good coverage for the happy paths! A few edge cases worth adding:
include: vec!["".to_string()]— the "disable defaults" sentinel actually disables detection- An invalid regex pattern in
include— verify it's gracefully skipped (not a panic) - A matching file that has no owning project — verify it doesn't crash
| @@ -1 +1 @@ | |||
| Subproject commit 0d6eb7a3c5c9e4e0e19ec9a0cd80e017c55a9c59 | |||
| Subproject commit 10ec84a6d723c01d9fbca21e983373b552ef5bd6 | |||
There was a problem hiding this comment.
Submodule pointer changed — what was added to the fixture monorepo? Please confirm this only contains test fixture changes needed for this PR and not unrelated modifications.
Summary
changedIncludedFilesPackagesmechanism so that changed files matching configurable regex patterns directly mark their owning project as affected\.(spec|test)\.(ts|js)x?$includeis empty (default), the default test file pattern is applied automaticallyincludeoption or CLI configRoot Cause
Traf has a
changedIncludedFilesPackagesstep that runs on ALL changed files before semantic analysis. Any file matching anincludepattern (default: test/spec files) immediately adds its owning project to the affected set. Domino had theincludefield inTrueAffectedConfigbut it was marked#[allow(dead_code)]and never used.This caused domino to miss projects where only test files changed, since test files may not be in the TypeScript project's
tsconfig.jsonand thus invisible to semantic analysis.Test plan
.spec.ts,.test.tsx, etc.) and non-matching files.spec.tsfile change marks owning project as affected (default pattern).stories.tsfile change with custom include patterncargo clippycleancargo fmtcleanMade with Cursor