Moved src/Assets to test/TestAssets #41924
Conversation
dotnet-issue-labeler
bot
added
Area-dotnet new
…ked up as unit test projects themselves.
…inor whitespace changes.
…e the testPackagesPath to a file for debugging.
…TestAssets. That was the problem. Removed the debugging exceptions.
|
FYI, when this goes to be merged, I will squash merge it to remove the debugging commits I made to find the LinkBase Asset rename I missed originally. |
| @@ -145,7 +139,7 @@ public static void Initialize(TestCommandLine commandLine) | |||
| { | |||
| testContext.TestExecutionDirectory = (Path.Combine(FindFolderInTree("artifacts", AppContext.BaseDirectory), "tmp", repoConfiguration)); | |||
|
|
|||
| testContext.TestAssetsDirectory = FindFolderInTree(Path.Combine("src", "Assets"), AppContext.BaseDirectory); | |||
| testContext.TestAssetsDirectory = FindFolderInTree(Path.Combine("test", "TestAssets"), AppContext.BaseDirectory); | |||
There was a problem hiding this comment.
Can we put test and TestAssets in constants somewhere. They appear more than once and someone is going to miss making a future change and only modify one occurrence.
There was a problem hiding this comment.
To be blunt, this code is confusing enough. I'd rather just write it logically to represent the scenarios that are necessary. Right now, I don't know those scenarios precisely without tearing everything apart and refactoring it.
That said, investing more right now into this file to "clean it up" isn't worth it. If you want me to do that, I'd rather do it in a separate PR. This PR is to simply move the folder. This entire file should have constants that represent the expected paths, not just test and TestAssets. For example, TestAssets exists in different locations and is used for different purposes. Adding constants for them doesn't actually represent the correct way this stuff is used. I can talk offline if you want about it, but it is also why it took me 3+ days to figure out 1 folder that was named wrong.
| @@ -79,7 +79,7 @@ | |||
|
|
|||
| <LockFiles Include="..\src\Tasks\Microsoft.NET.Build.Tasks.UnitTests\LockFiles\**\*.*" /> | |||
|
|
|||
| <AssetFiles Include="..\src\Assets\**\**\**\*.*" /> | |||
| <AssetFiles Include="TestAssets\**\*.*" /> | |||
There was a problem hiding this comment.
Should we consider putting this behind a $(TestAssets) property?
There was a problem hiding this comment.
I think this is fine for now. The design of TestAssets is actually incorrect to begin with. It assumes that all test projects require ALL TestAssets, which is wrong. So, we do a lot of duplicate file copying. I'd have to refactor the test projects to make sense. I've discussed this with Marc, but it isn't something I can focus effort on right now.
Summary
From when
src/Testswas moved totest, I did not realize at that time that everything insrc/Assetsare also only test assets. I ran into this situation when attempting to get the Installer tests running in Helix, as their test assets were intest/TestAssets. So, this PR is to merge both together intotest/TestAssets.A majority of the files were moved over with no changes. Some files were deleted as those existed in both locations. Therefore, I just moved the changes from the 'newer'
src/Assetsfiles into the ones that already existed. You'll also see files being deleted and the new files being created if the folder already existed in the destination. Otherwise, everything else was just a simple 'rename'.Manual File Changes