Use MoveFile and remove CopyFile/DeleteFile#7506
Use MoveFile and remove CopyFile/DeleteFile#7506SimonCropp wants to merge 1 commit intomicrosoft:mainfrom
Conversation
Unify file-moving logic by removing the NETCOREAPP conditional and calling IFileSystem.MoveFile unconditionally in RetryOrchestrator. Remove CopyFile and DeleteFile members from IFileSystem and their implementations in SystemFileSystem, simplifying the file system abstraction.
There was a problem hiding this comment.
Pull request overview
This PR simplifies the platform file system abstraction and unifies result-asset “move” behavior in the Retry extension by always using IFileSystem.MoveFile (removing the previous CopyFile/DeleteFile fallback).
Changes:
- Removed
CopyFileandDeleteFilefromMicrosoft.Testing.Platform.Helpers.IFileSystem. - Removed the corresponding implementations from
SystemFileSystem. - Updated
RetryOrchestratorto callMoveFile(..., overwrite: true)unconditionally (removed#if NETCOREAPPsplit).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/Platform/Microsoft.Testing.Platform/Helpers/System/SystemFileSystem.cs | Drops CopyFile/DeleteFile implementation in line with the interface simplification. |
| src/Platform/Microsoft.Testing.Platform/Helpers/System/IFileSystem.cs | Removes unused CopyFile/DeleteFile members to reduce abstraction surface area. |
| src/Platform/Microsoft.Testing.Extensions.Retry/RetryOrchestrator.cs | Uses MoveFile consistently for moving attempt assets to the final results directory. |
| void CopyFile(string sourceFileName, string destFileName, bool overwrite = false); | ||
|
|
||
| void DeleteFile(string path); |
There was a problem hiding this comment.
This is a binary breaking change. It's fine to simplify the RetryOrchestrator, but we shouldn't remove these members from the interface, at least for few minor versions.
There was a problem hiding this comment.
We should also add a comment explicitly stating that while these are unused, they are needed so that older versions of Retry can remain compatible with newer versions of MTP.
There was a problem hiding this comment.
my understanding is that all nugets are deployed at the same time. given that they must have a min version on the core. so it should be safe to remove internal APIs. unless there are external projects that use internals and are deployed in an out of sync way?
There was a problem hiding this comment.
We deploy at the same time, and we have min version. But given that the version constraint is "min" and not "strictly equal", this is problematic.
Consider:
- Project uses MSTest 4.1 (bringing MTP 2.1)
- Project uses Retry 2.1
- Project uses TRX 2.1
Then after our next release, if the user updated TRX to 2.2. They will hit an issue.
The Retry version being used is 2.1, while the core MTP used is 2.2 (brought via TRX). Retry 2.1 will attempt to access CopyFile/DeleteFile, but these will not be present at runtime in 2.2.
Unify file-moving logic by removing the NETCOREAPP conditional and calling IFileSystem.MoveFile unconditionally in RetryOrchestrator. Remove CopyFile and DeleteFile members from IFileSystem and their implementations in SystemFileSystem, simplifying the file system abstraction.