Add workload garbage collection tests for side-by-side manifests #35857
Conversation
dotnet-issue-labeler
bot
added
Area-Workloads
untriaged
dsplaisted
force-pushed
the
side-by-side-gc-tests
branch
from
985930f
to
33ca6b4
Compare
src/Tests/dotnet-workload-install.Tests/WorkloadGarbageCollectionTests.cs
Show resolved
Hide resolved
| } | ||
| } | ||
|
|
||
| private void CreateInstallState(string featureBand, string installStateContents) |
There was a problem hiding this comment.
Some of these functions have been duplicated a lot across tests, such as this one. There is a function CreateMockInstallState in SdkDirectoryWorkloadManifestProviderTests which is nearly identical, however, it does not use the featureBand parameter correctly. I do understand the simplicity of using these functions per test file, though I think it would be best for maintenance's sake to try to coalesce them into one WorkloadTestUtilities.cs file
There was a problem hiding this comment.
Yes, I copied this from SdkDirectoryWorkloadManifestProviderTests, and noticed the bug so I fixed it in this PR.
I think most of the helper methods use private fields of the class so that they're less verbose to call. So it's not so simple to factor them out into a shared file, I think you'd need a base class for workload tests. I'd be happy to see someone try that, but I didn't want to tackle that here.
Overall, I don't like duplicated code, but I think I'm a lot more OK with it in tests.
There was a problem hiding this comment.
Ah, I did not even notice that you had fixed CreateMockInstallState. I would also be happy to have some of this compressed down together, though it doesn't have to be in this PR. I will give it a 👍, though I would be more pleased once these get cleaned up in the future ;)
| { | ||
| foreach (var pack in packsToKeep.Concat(packsToCollect)) | ||
| { | ||
| CreateInstalledPack(pack, sdkVersion); |
There was a problem hiding this comment.
I see you have cleaned up the tests quite a bit, I quite like this CreateInstalledPack function to de-duplicate the logic. 😄
src/Tests/dotnet-workload-install.Tests/WorkloadGarbageCollectionTests.cs
Outdated
Show resolved
Hide resolved
src/Tests/dotnet-workload-install.Tests/WorkloadGarbageCollectionTests.cs
Show resolved
Hide resolved
src/Tests/dotnet-workload-install.Tests/WorkloadGarbageCollectionTests.cs
Outdated
Show resolved
Hide resolved
src/Tests/dotnet-workload-install.Tests/WorkloadGarbageCollectionTests.cs
Show resolved
Hide resolved
src/Tests/dotnet-workload-install.Tests/WorkloadGarbageCollectionTests.cs
Outdated
Show resolved
Hide resolved
src/Tests/dotnet-workload-install.Tests/WorkloadGarbageCollectionTests.cs
Outdated
Show resolved
Hide resolved
| return new PackInfo(new WorkloadPackId(id), version, kind, path, resolvedPackageId); | ||
| } | ||
|
|
||
| FileInfo PackRecord(PackInfo pack, string sdkFeatureBand) |
There was a problem hiding this comment.
Should we parameterize the v1 and default it to "v1" in case we ever need v2?
There was a problem hiding this comment.
I don't think so. The v1 in the path is a future-proofing mechanism that we may never need. If we do need it, then we would probably be changing more about how the records are stored, so the test code would have to update anyway with more than just a change from v1 to v2.
No description provided.