[browser] Move boot config tests to Wasm.Build.Tests#131223
Conversation
When RuntimeConfigJsonPath is set and IsPublish is false, also check for a runtimeconfig.dev.json file alongside the main config. If found, merge its configProperties into the result, with dev values overriding main config values. This allows the SDK to use runtimeconfig.dev.json to set Hot Reload related switches in debug builds. Fixes #130823
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: b49d3786-9f03-4fd3-8076-28a73d639686
Minor formatting-only changes in `src/coreclr/jit/rangecheck.h` to wrap long lines. No behavioral changes.
Move GenerateWasmBootJson test coverage from the temporary tasks.tests project into Wasm.Build.Tests, remove the obsolete tasks.tests csproj, and tag the moved tests as no-workload. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2db6a9dd-0c66-4a48-b2f6-36c7329a5e0e
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @akoeplinger, @matouskozak, @simonrozsival |
There was a problem hiding this comment.
Pull request overview
This PR consolidates GenerateWasmBootJson unit tests into the main Wasm.Build.Tests test project and removes the now-redundant temporary Pack.Tasks.Tests test project, keeping WebAssembly SDK task testing in one place.
Changes:
- Moved
GenerateWasmBootJsonTestsintosrc/mono/wasm/Wasm.Build.Tests. - Removed the old
src/tasks.tests/Microsoft.NET.Sdk.WebAssembly.Pack.Tasks.Teststest project and its test file. - Removed the obsolete
InternalsVisibleToentry fromMicrosoft.NET.Sdk.WebAssembly.Pack.Tasks.csproj.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/tasks/Microsoft.NET.Sdk.WebAssembly.Pack.Tasks/Microsoft.NET.Sdk.WebAssembly.Pack.Tasks.csproj | Removes InternalsVisibleTo for the deleted Pack.Tasks.Tests project. |
| src/tasks.tests/Microsoft.NET.Sdk.WebAssembly.Pack.Tasks.Tests/Microsoft.NET.Sdk.WebAssembly.Pack.Tasks.Tests.csproj | Deletes the temporary test project. |
| src/tasks.tests/Microsoft.NET.Sdk.WebAssembly.Pack.Tasks.Tests/GenerateWasmBootJsonTests.cs | Deletes the old test implementation from the temporary project. |
| src/mono/wasm/Wasm.Build.Tests/GenerateWasmBootJsonTests.cs | Adds the moved test class under Wasm.Build.Tests (including no-workload categorization). |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 543feee9-af37-42d8-8313-055510ee76d9
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 543feee9-af37-42d8-8313-055510ee76d9
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
src/mono/wasm/Wasm.Build.Tests/GenerateWasmBootJsonTests.cs:187
GenerateWasmBootJson.ReadRuntimeConfigFilesisinternal, and this repo already grants internals access to Wasm.Build.Tests viaInternalsVisibleToplus aProjectReference. Using reflection here makes the test more brittle (name-based lookup, BindingFlags constraints, exceptions wrapped in TargetInvocationException) and hides compile-time signature changes. Prefer a direct call to the internal method instead of reflection.
{
public string Path { get; } = System.IO.Path.Combine(System.IO.Path.GetTempPath(), System.IO.Path.GetRandomFileName());
public TempDirectory() => Directory.CreateDirectory(Path);
public void Dispose()
{
try
{
Directory.Delete(Path, recursive: true);
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/tasks/Microsoft.NET.Sdk.WebAssembly.Pack.Tasks/Microsoft.NET.Sdk.WebAssembly.Pack.Tasks.csproj:21
- PR description says the Pack.Tasks csproj removes an obsolete InternalsVisibleTo, but this change actually updates it to point to Wasm.Build.Tests (and it’s still needed for the moved tests to call the internal ReadRuntimeConfigFiles). Please update the PR description to reflect that the friend assembly was changed rather than removed, to avoid confusion for reviewers/backports.
<ItemGroup>
<InternalsVisibleTo Include="Wasm.Build.Tests" />
<Compile Include="..\Common\Utils.cs" />
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 543feee9-af37-42d8-8313-055510ee76d9
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
src/mono/wasm/Wasm.Build.Tests/GenerateWasmBootJsonTests.cs:193
- InvokeReadRuntimeConfigFiles uses Type.GetMethod(name, flags) without specifying parameter types. If GenerateWasmBootJson ever gets another overload named ReadRuntimeConfigFiles, this will throw AmbiguousMatchException (bypassing the intended error message) or bind to an unexpected overload. Prefer the GetMethod overload that includes the expected parameter types so the failure mode is deterministic.
private static RuntimeConfigData? InvokeReadRuntimeConfigFiles(string? mainConfigPath, string? devConfigPath)
{
MethodInfo? method = typeof(GenerateWasmBootJson).GetMethod(
"ReadRuntimeConfigFiles",
BindingFlags.Static | BindingFlags.NonPublic);
if (method is null)
{
throw new InvalidOperationException(
$"{nameof(GenerateWasmBootJson)}.{nameof(InvokeReadRuntimeConfigFiles)} could not find private method " +
"'ReadRuntimeConfigFiles(string?, string?)'. The production method signature may have changed.");
}
object? result = method.Invoke(null, new object?[] { mainConfigPath, devConfigPath });
return (RuntimeConfigData?)result;
}
src/mono/wasm/Wasm.Build.Tests/GenerateWasmBootJsonTests.cs:216
- TempDirectory.Dispose currently only swallows a small set of exceptions. If Directory.Delete throws other exceptions (e.g., PathTooLongException, SecurityException, ArgumentException), the test can fail during cleanup and mask the real assertion failure/success. For temp test directories, it’s typically safer to swallow all cleanup exceptions (ideally with a brief comment) to reduce flakiness.
public void Dispose()
{
try
{
Directory.Delete(Path, recursive: true);
}
catch (DirectoryNotFoundException)
{
}
catch (IOException)
{
}
catch (UnauthorizedAccessException)
{
}
}
src/tasks/Microsoft.NET.Sdk.WebAssembly.Pack.Tasks/Microsoft.NET.Sdk.WebAssembly.Pack.Tasks.csproj:24
- GenerateWasmBootJson.ReadRuntimeConfigFiles is internal. The moved tests currently use reflection to call it, which loses compile-time checking and can fail at runtime if overloads/signatures change. Consider adding an InternalsVisibleTo for Wasm.Build.Tests and calling the method directly, or otherwise justify preferring reflection over a friend-assembly relationship for tests.
<ItemGroup>
<Compile Include="..\Common\Utils.cs" />
<Compile Include="..\WasmAppBuilder\WebcilConverter.cs" />
<Compile Include="..\WasmAppBuilder\LogAdapter.cs" />
<ProjectReference Include="..\Microsoft.NET.WebAssembly.Webcil\Microsoft.NET.WebAssembly.Webcil.csproj" />
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 543feee9-af37-42d8-8313-055510ee76d9
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
src/mono/wasm/Wasm.Build.Tests/GenerateWasmBootJsonTests.cs:18
- The new test file is indented as if the file-scoped namespace had a block (the class and all members are shifted right). This is inconsistent with other Wasm.Build.Tests files that use file-scoped namespaces and will make future diffs noisy. Please reformat the file so the class starts at column 0 under
namespace Wasm.Build.Tests;(and reindent the body accordingly).
namespace Wasm.Build.Tests;
[TestCategory("no-workload")]
public class GenerateWasmBootJsonTests
{
src/mono/wasm/Wasm.Build.Tests/GenerateWasmBootJsonTests.cs:184
- The reflection lookup for
ReadRuntimeConfigFilesis brittle: it only searchesNonPublicmethods and doesn't specify parameter types, so it will fail if the method ever becomes public or gains overloads (AmbiguousMatchException), even though the test could still work. Also the exception message says "private" but the method is currentlyinternal.
Consider making the lookup tolerant by including BindingFlags.Public and matching the exact parameter types, and adjust the message to say "non-public".
MethodInfo? method = typeof(GenerateWasmBootJson).GetMethod(
"ReadRuntimeConfigFiles",
BindingFlags.Static | BindingFlags.NonPublic);
if (method is null)
Move GenerateWasmBootJson tests from the temporary
src/tasks.tests/Microsoft.NET.Sdk.WebAssembly.Pack.Tasks.Testsproject intosrc/mono/wasm/Wasm.Build.Tests.Changes:
GenerateWasmBootJsonTestsinWasm.Build.Tests[TestCategory("no-workload")]to the moved test classPack.Tasks.Teststest file and csprojInternalsVisibleTofromMicrosoft.NET.Sdk.WebAssembly.Pack.Tasks.csprojNote
This PR description was generated with GitHub Copilot assistance.