Fix merge conflict checker for empty file lists and filter *.cs files #26365
Conversation
- Remove mandatory parameter constraint on $File in Test-MergeConflictMarker - Add [AllowEmptyCollection()] attribute and default empty array - Handle empty file list gracefully (e.g., when PR only deletes files) - Filter out *.cs files from merge conflict checking - Ensure action wraps ConvertFrom-Json result in @() to handle empty JSON arrays - Update README to document new behavior Co-authored-by: TravisEz13 <[email protected]>
Extract common output writing logic into a helper scriptblock to eliminate duplication between empty file list and filtered file list cases. Co-authored-by: TravisEz13 <[email protected]>
Changed $writeNoFilesOutput from a scriptblock variable to a proper nested function Write-NoFilesOutput with explicit parameters for better code clarity and maintainability. Co-authored-by: TravisEz13 <[email protected]>
|
@copilot You didn't run the tests and they are failing. I think the test's expectation needs to be updated. Running tests from '/home/runner/work/PowerShell/PowerShell/test/infrastructure/ciModule.Tests.ps1' Summary: |
Updated test to match new behavior where empty arrays are handled gracefully instead of throwing an error. This supports delete-only PRs. The test now verifies: - Empty arrays are accepted without error - Proper GitHub Actions outputs are generated (files-checked=0, conflicts-found=0) - Summary contains "No Files to Check" message Co-authored-by: TravisEz13 <[email protected]>
Fixed the test expectations. The test was checking that empty arrays throw an error, but the fix was specifically to handle empty arrays gracefully (for delete-only PRs). Updated the test to verify the new behavior: accepts empty arrays and generates proper outputs. All 19 tests now pass. (24b4361) |
TravisEz13
added
CL-Test
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the Test-MergeConflictMarker function to handle edge cases and adds filtering for C# files. The changes make the function more robust for scenarios like delete-only PRs and exclude *.cs files from merge conflict checking.
- The
$Fileparameter now accepts empty arrays with[AllowEmptyCollection()]attribute and has a default value of@() - Added filtering to exclude
*.csfiles from merge conflict detection - Introduced a helper function
Write-NoFilesOutputfor consistent output when no files need checking
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tools/ci.psm1 | Modified Test-MergeConflictMarker to handle empty file lists, filter out *.cs files, and generate appropriate output for these scenarios |
| test/infrastructure/ciModule.Tests.ps1 | Updated test to verify graceful handling of empty file arrays instead of expecting an exception |
| .github/actions/infrastructure/merge-conflict-checker/action.yml | Wrapped ConvertFrom-Json result in array syntax to ensure $changedFiles is always an array |
| .github/actions/infrastructure/merge-conflict-checker/README.md | Added documentation about C# file filtering and empty file list handling |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Get changed files from environment variable (secure against injection) | ||
| $changedFilesJson = $env:CHANGED_FILES_JSON | ||
| $changedFiles = $changedFilesJson | ConvertFrom-Json | ||
| # Ensure we always have an array (ConvertFrom-Json returns null for empty JSON arrays) |
There was a problem hiding this comment.
The comment states "ConvertFrom-Json returns null for empty JSON arrays" but this is inaccurate. ConvertFrom-Json with an empty JSON array [] returns an empty array, not null. It returns $null only when the input is the JSON literal "null" or an empty/null string. The array wrapper is still useful for ensuring consistent array behavior, but the comment should be corrected.
| # Ensure we always have an array (ConvertFrom-Json returns null for empty JSON arrays) | |
| # Ensure we always have an array. ConvertFrom-Json returns an empty array for '[]', and $null only for the JSON literal 'null' or an empty/null string. |
|
📣 Hey @@Copilot, how did we do? We would love to hear your feedback with the link below! 🗣️ 🔗 https://aka.ms/PSRepoFeedback |
…PowerShell#26365) Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: TravisEz13 <[email protected]>
…PowerShell#26365) Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: TravisEz13 <[email protected]>
…PowerShell#26365) Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: TravisEz13 <[email protected]>
…PowerShell#26365) Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: TravisEz13 <[email protected]>
Fix CI Failure in Merge Conflict Checker
Problem
Test-MergeConflictMarkerhas a mandatory[Parameter(Mandatory)]on the$FileparameterSolution
Test-MergeConflictMarkerto handle empty file arrays gracefully[Parameter(Mandatory)]constraint[AllowEmptyCollection()]attributeTest-MergeConflictMarkerfunctionConvertFrom-Jsonresult in@()to ensure always an arrayChanges Made
tools/ci.psm1:
Test-MergeConflictMarkerfunction to accept empty file arraysWrite-NoFilesOutput.github/actions/infrastructure/merge-conflict-checker/action.yml:
ConvertFrom-Jsonresult in@()to handle empty JSON arrays properly.github/actions/infrastructure/merge-conflict-checker/README.md:
test/infrastructure/ciModule.Tests.ps1:
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.