[release/v7.5] Fix merge conflict checker for empty file lists and filter *.cs files#26813
Conversation
…PowerShell#26365) Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: TravisEz13 <[email protected]>
daxian-dbw
added
the
CL-Test
There was a problem hiding this comment.
Pull request overview
This PR is a backport of #26365 to the release/v7.5 branch, fixing a critical CI failure in the merge conflict checker that occurred when a pull request only deleted files (resulting in an empty file array). The fix enhances the robustness of the CI infrastructure by gracefully handling edge cases and adding file filtering capabilities.
Changes:
- Added new
Test-MergeConflictMarkerfunction toci.psm1with support for empty file arrays and *.cs file filtering - Created comprehensive test suite for merge conflict detection in
ciModule.Tests.ps1 - Implemented GitHub Actions composite action for merge conflict checking with proper empty array handling
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| tools/ci.psm1 | Added Test-MergeConflictMarker function with empty array handling, *.cs filtering, and GitHub Actions output generation; minor whitespace cleanup |
| test/infrastructure/ciModule.Tests.ps1 | New comprehensive test suite covering empty arrays, conflict detection, edge cases, and Install-CIPester function tests |
| .github/actions/infrastructure/merge-conflict-checker/action.yml | New composite action that wraps ConvertFrom-Json in @() to handle empty JSON arrays |
| .github/actions/infrastructure/merge-conflict-checker/README.md | Documentation for the new action including usage, behavior, and examples |
|
|
||
| It "Should successfully run without errors when Pester exists" { | ||
| if (!$env:CI) { | ||
| Set-ItResult -Skipped -Because "Test requires CI environment to safely install Pester" |
There was a problem hiding this comment.
According to the Pester Set-ItResult pattern convention in this repository, when using Set-ItResult with -Skipped, you must immediately follow it with a return statement to prevent further test execution. Without the return, the test body continues to execute and may produce unexpected behavior or errors. Add return statements after both Set-ItResult calls.
|
|
||
| It "Should accept custom version parameters" { | ||
| if (!$env:CI) { | ||
| Set-ItResult -Skipped -Because "Test requires CI environment to safely install Pester" |
There was a problem hiding this comment.
According to the Pester Set-ItResult pattern convention in this repository, when using Set-ItResult with -Skipped, you must immediately follow it with a return statement to prevent further test execution. Without the return, the test body continues to execute and may produce unexpected behavior or errors. Add a return statement after this Set-ItResult call.
|
|
||
| ``` | ||
| ❌ Merge conflict markers detected in the following files: | ||
| - src/example.cs |
There was a problem hiding this comment.
The example output shows src/example.cs as a file with conflict markers, but according to line 59, *.cs files are filtered out and excluded from merge conflict checking. This creates a contradiction in the documentation. Update the example to use a non-CS file, such as src/example.ps1 or src/example.txt.
| - src/example.cs | |
| - src/example.ps1 |
| $filesChecked++ | ||
|
|
||
| # Get relative path for display | ||
| $relativePath = if ($WorkspacePath -and $filePath.StartsWith($WorkspacePath)) { |
There was a problem hiding this comment.
The StartsWith method should use case-insensitive comparison for cross-platform path compatibility. According to codebase conventions, use [System.StringComparison]::OrdinalIgnoreCase for path comparisons. Change to: $filePath.StartsWith($WorkspacePath, [System.StringComparison]::OrdinalIgnoreCase)
| $relativePath = if ($WorkspacePath -and $filePath.StartsWith($WorkspacePath)) { | |
| $relativePath = if ($WorkspacePath -and $filePath.StartsWith($WorkspacePath, [System.StringComparison]::OrdinalIgnoreCase)) { |
| $summary | Should -Match "\| Line \| Marker \|" | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing test coverage for the *.cs file filtering behavior, which is one of the key features of this PR. Add a test case that verifies *.cs files are filtered out from merge conflict checking. For example, test that when a .cs file with conflict markers is provided, it should be skipped and the function should report 0 files checked and 0 conflicts found.
Backport of #26365 to release/v7.5
Triggered by @daxian-dbw on behalf of @app/copilot-swe-agent
Original CL Label: CL-Test
/cc @PowerShell/powershell-maintainers
Impact
REQUIRED: Choose either Tooling Impact or Customer Impact (or both). At least one checkbox must be selected.
Tooling Impact
Fixes merge conflict checker to handle empty file lists and filter *.cs files
Customer Impact
Regression
REQUIRED: Check exactly one box.
This is not a regression.
Testing
Updated infrastructure tests verify the fix. Successfully tested in 7.4 and 7.6 releases. Handles empty file arrays gracefully.
Risk
REQUIRED: Check exactly one box.
Fixes a bug in the merge conflict checker where empty file lists cause CI failures. Adds filtering for *.cs files. Successfully backported to 7.4 and 7.6 branches.
Merge Conflicts
Conflicts in merge-conflict-checker action files, ciModule.Tests.ps1, and ci.psm1 resolved - context differences from previous backports