Simplify LocateRepository relative-path test (follow-up to #1734)#1737
Closed
jankratochvilcz wants to merge 10 commits into
Closed
Simplify LocateRepository relative-path test (follow-up to #1734)#1737jankratochvilcz wants to merge 10 commits into
jankratochvilcz wants to merge 10 commits into
Conversation
Migrates the shared abstract base RepositoryTask and the concrete
LocateRepository task to the MSBuild multithreaded (MT) task model.
Base RepositoryTask:
- Implements IMultiThreadableTask with a TaskEnvironment property that
defaults to TaskEnvironment.Fallback so existing call paths and unit
tests keep single-process (CWD) semantics with no setup.
- Absolutizes the initial path at the GetInitialPath() boundary:
GetOrCreateRepositoryInstance now computes
TaskEnvironment.GetAbsolutePath(initialPath) and passes the resolved
AbsolutePath.Value to GitRepository.TryFindRepository. This removes the
process-CWD dependency (TryFindRepository internally calls
Path.GetFullPath, which only consults the CWD for relative inputs;
feeding it an absolute path is MT-safe).
Note: the implicit AbsolutePath->string conversion returns the original
(possibly relative) input, so .Value is used explicitly.
- Sin 2: both ReportMissingRepositoryWarning call sites keep the ORIGINAL
initialPath so warnings show the user's input, not the absolutized path.
- Edge case: GetAbsolutePath throws ArgumentException on null/empty,
matching the previous Path.GetFullPath("") behavior.
Concrete LocateRepository:
- Annotated with [MSBuildMultiThreadableTask].
Call-chain audit: GitOperations.GetRepositoryUrl / GetSourceRoots and the
downstream GitRepository path operations combine paths with the absolute
git/working directories of the located repository, so they become MT-safe
once the initial path is absolute. No remaining
Directory.GetCurrentDirectory / Environment.CurrentDirectory /
Environment.Get/SetEnvironmentVariable / CWD-relative Path.GetFullPath in
the LocateRepository chain.
Test:
- Adds LocateRepositoryTests.RelativePath_ResolvesAgainstTaskEnvironment-
ProjectDirectory_NotProcessCwd (Pattern A decoy CWD): creates a real
minimal git repo, sets the process CWD to a repo-less decoy, sets the
task's TaskEnvironment project directory to the repo, passes a relative
Path, and asserts WorkingDirectory/RepositoryId resolve to the project
repo. Fails if the migration is reverted.
- Extends MockEngine to IBuildEngine4 (in-memory registered task object
store) so the cached success path can run.
- Disables assembly test parallelization to keep the CWD mutation safe.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The implicit string conversion returns the absolute Value, not the original input; correct the comment while keeping the explicit .Value. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Wrap GetAbsolutePath + TryFindRepository so an empty/null initial path reports a missing-repository warning and returns gracefully instead of throwing an unhandled ArgumentException (preserves pre-migration behavior; ExecuteImpl's catch does not handle ArgumentException). - Rewrite the parity comment to describe the control-flow (not just exception type) equivalence with the old TryFindRepository path. - Add EmptyPath_DegradesGracefully regression test (dual-fault verified: fails when the catch is reverted). - Make AssemblyInfo.cs parallelization comment test-agnostic. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…itory) Make the MT migration attribute-only: rather than resolving the initial path via TaskEnvironment.GetAbsolutePath, RepositoryTask now rejects any path that is not fully qualified. A relative/drive-relative/root-relative path depends on the shared process CWD/drive, which is unsafe under the multithreaded task model, so it degrades to the standard "missing repository" warning. - Add IsPathFullyQualified polyfill to PathUtilities (net472 lacks the BCL API) with a dedicated test suite (Windows/Unix/relative cases). - Drop IMultiThreadableTask/TaskEnvironment from RepositoryTask; keep [MSBuildMultiThreadableTask] on the concrete LocateRepository. - Simplify tests: revert shared MockEngine to IBuildEngine; localize a minimal IBuildEngine4 to LocateRepositoryTests. Cover historical behavior (absolute path locates repo) and CWD-independence (relative path rejected even when CWD is a repo). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Move the inline IBuildEngine4 test engine into a shared TestUtilities.MockEngine4 next to the existing MockEngine so other tests can reuse it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…f new MockEngine4 Per review feedback, the existing MockEngine in TestUtilities now implements IBuildEngine4 (adding the IBuildEngine2/3/4 members and an in-memory registered-task-object store) rather than introducing a separate MockEngine4 test double. IBuildEngine4 derives from IBuildEngine, so all existing MockEngine usages remain source-compatible. - Extend MockEngine : IBuildEngine -> IBuildEngine4 - Delete MockEngine4.cs - Update LocateRepositoryTests to use MockEngine Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Per review feedback, the RelativePath test no longer sets the process
current working directory. It is sufficient to validate that a
non-fully-qualified path is rejected, so the test just passes a relative
path (".") and asserts the missing-repository warning.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
Author
|
Closing as redundant. #1734 merged with the test simplification (commit 8e9e319) as its final commit, so this change is already in |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #1734 addressing review comment https://github.com/dotnet/sourcelink/pull/1734/changes#r3537777727.
The reviewer noted we don't need to set the process CWD in the relative-path test — it is sufficient to validate that a non-fully-qualified argument is rejected.
This change (
src/Microsoft.Build.Tasks.Git.UnitTests/LocateRepositoryTests.cs):Directory.SetCurrentDirectorymanipulation (and the try/finally restore) from the relative-path test.RelativePath_IsRejected_IndependentOfProcessCwdtoRelativePath_IsRejected; it now just passes a relative path (".") and asserts the missing-repository warning.System.IOusing.Note
This PR is stacked on #1734. Until #1734 merges, the diff here also shows #1734's commits; it will narrow to just the test simplification once #1734 is in
main.Verified:
RelativePath_IsRejectedandAbsolutePath_LocatesRepositorypass on net11.0.