Skip to content

Conversation

@elinor-fung
Copy link
Member

I realized we didn't have tests around serviceable assets when doing the .deps.json relative local path work. My initial plan for that would have broken these scenarios - hopefully having tests like these would have made that clearer earlier on.

cc @dotnet/appmodel @AaronRobinsonMSFT

Copilot AI review requested due to automatic review settings August 26, 2025 18:28
@elinor-fung elinor-fung added test-enhancement Improvements of test source code area-Host labels Aug 26, 2025
@dotnet-policy-service
Copy link
Contributor

Tagging subscribers to this area: @vitek-karas, @agocke, @VSadov
See info in area-owners.md if you want to be subscribed.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds comprehensive host tests for serviceable assets in the .NET Core runtime. The tests verify that the host correctly resolves serviceable assets from the servicing directory while non-serviceable assets are resolved from the application directory.

  • Extends the test framework to support configuring serviceable properties on libraries
  • Adds three test cases covering runtime assemblies, native libraries, and resource assemblies
  • Tests are platform-specific (skipped on Windows due to servicing location requirements)

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
src/installer/tests/TestUtils/NetCoreAppBuilder.cs Adds serviceable property support to RuntimeLibraryBuilder with a new WithServiceable() method
src/installer/tests/HostActivation.Tests/DependencyResolution/ServiceableAssets.cs New test file containing comprehensive serviceable asset resolution tests

public string Name { get; }
public string Version { get; }
public string Path { get; }
public string ServicedPath { get; set; }
Copy link

Copilot AI Aug 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ServicedPath property is mutable but lacks validation. Consider making it private set or adding validation to ensure it's only set once by SetUpServicingDirectory.

Suggested change
public string ServicedPath { get; set; }
public string ServicedPath { get; private set; }
public void SetServicedPath(string path)
{
if (ServicedPath != null)
{
throw new InvalidOperationException("ServicedPath can only be set once.");
}
ServicedPath = path;
}

Copilot uses AI. Check for mistakes.
Comment on lines +163 to +174
private string SetUpServicingDirectory(string directory, params LibraryAsset[] libraryAssets)
{
string servicingDir = Path.Combine(directory, "servicing");
foreach (var asset in libraryAssets)
{
string servicedAsset = Path.Combine(servicingDir, "pkgs", asset.Name, asset.Version, asset.Path);
Directory.CreateDirectory(Path.GetDirectoryName(servicedAsset));
File.WriteAllText(servicedAsset, string.Empty);
asset.ServicedPath = servicedAsset;
}

return servicingDir;
Copy link

Copilot AI Aug 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mutating the ServicedPath property of the input parameter creates a side effect that may not be obvious to callers. Consider returning a modified copy or using a different design pattern.

Suggested change
private string SetUpServicingDirectory(string directory, params LibraryAsset[] libraryAssets)
{
string servicingDir = Path.Combine(directory, "servicing");
foreach (var asset in libraryAssets)
{
string servicedAsset = Path.Combine(servicingDir, "pkgs", asset.Name, asset.Version, asset.Path);
Directory.CreateDirectory(Path.GetDirectoryName(servicedAsset));
File.WriteAllText(servicedAsset, string.Empty);
asset.ServicedPath = servicedAsset;
}
return servicingDir;
private (string servicingDir, LibraryAsset servicedServiceableLib, LibraryAsset servicedNonServiceableLib)
SetUpServicingDirectory(string directory, LibraryAsset serviceableLib, LibraryAsset nonServiceableLib)
{
string servicingDir = Path.Combine(directory, "servicing");
string servicedServiceableAsset = Path.Combine(servicingDir, "pkgs", serviceableLib.Name, serviceableLib.Version, serviceableLib.Path);
Directory.CreateDirectory(Path.GetDirectoryName(servicedServiceableAsset));
File.WriteAllText(servicedServiceableAsset, string.Empty);
string servicedNonServiceableAsset = Path.Combine(servicingDir, "pkgs", nonServiceableLib.Name, nonServiceableLib.Version, nonServiceableLib.Path);
Directory.CreateDirectory(Path.GetDirectoryName(servicedNonServiceableAsset));
File.WriteAllText(servicedNonServiceableAsset, string.Empty);
var servicedServiceableLib = new LibraryAsset(serviceableLib.Name, serviceableLib.Path)
{
Version = serviceableLib.Version,
ServicedPath = servicedServiceableAsset
};
var servicedNonServiceableLib = new LibraryAsset(nonServiceableLib.Name, nonServiceableLib.Path)
{
Version = nonServiceableLib.Version,
ServicedPath = servicedNonServiceableAsset
};
return (servicingDir, servicedServiceableLib, servicedNonServiceableLib);

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-Host test-enhancement Improvements of test source code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant