-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Re-enable SB tests #46055
Re-enable SB tests #46055
Conversation
Current test failures: DotNetWatchTests.WatchTests
WebScenarioTests.VerifyScenario
DotNetFormatTests.FormatProject
SymbolsTests.VerifySdkSymbols
|
@NikolaMilosavljevic - Could you help investigate why the symbols test is failing? It looks like all of the DLLs within the SDK are being reported as having no PDBs. |
Pushed a fix for symbols tests - 196f148 |
sdk/src/SourceBuild/content/test/Microsoft.DotNet.SourceBuild.Tests/DotNetWatchTests.cs Line 39 in 2f9c4fe
should be changed to: const string waitingString = "Waiting for a file to change before restarting";
The @@ -1,22 +1,17 @@
{
"runtimeTarget": {
- "name": ".NETCoreApp,Version=v9.0",
+ "name": ".NETCoreApp,Version=v10.0",
"signature": ""
},
"compilationOptions": {},
"targets": {
- ".NETCoreApp,Version=v9.0": {
- "Microsoft.CodeAnalysis.Workspaces.MSBuild.BuildHost/4.12.0-3.24570.6": {
+ ".NETCoreApp,Version=v10.0": {
+ "Microsoft.CodeAnalysis.Workspaces.MSBuild.BuildHost/4.14.0-1.25064.9": {
"dependencies": {
- "Microsoft.Build": "17.3.4",
"Microsoft.Build.Framework": "17.3.4",
- "Microsoft.Build.Locator": "1.6.10",
- "Microsoft.Build.Tasks.Core": "17.3.4",
- "Microsoft.Build.Utilities.Core": "17.3.4",
- "Microsoft.DotNet.XliffTasks": "9.0.0-beta.24516.2",
"Newtonsoft.Json": "13.0.3",
- "System.Collections.Immutable": "9.0.0",
- "System.CommandLine": "2.0.0-beta4.24326.1"
+ "System.Collections.Immutable": "10.0.0-alpha.1.25063.12",
+ "System.CommandLine": "2.0.0-beta4.24529.1"
},
"runtime": {
"Microsoft.CodeAnalysis.Workspaces.MSBuild.BuildHost.dll": {}
@@ -63,61 +58,9 @@
}
}
}, |
My limited understanding is that this functionality is different than the ASP.NET Core runtime, which is what the test is checking for. It would be good to understand what it is exactly and how this is going to be delivered eventually. |
@MackinnonBuck - Can you provide some more context around the Microsoft.AspNetCore.App.Internal.Assets package and its inclusion with the SDK changes in #44997? We have a test that is failing due to this assembly. The test runs a self-contained publish of a web project. The intent of the test is to verify that no packages get restored from a NuGet feed and instead are available directly from the SDK. But that doesn't happen with the Microsoft.AspNetCore.App.Internal.Assets package. |
That package contains JavaScript files that get served as part of each Blazor app to provide core framework functionality. Prior to the introduction of this package, these files were embedded resources in framework DLLs, but this meant they couldn't participate in the app's build process the same way as other assets defined by the app (build time compression and fingerprinting, etc.). Furthermore, NodeJS is required to build these files, but we didn't want NodeJS to be a dependency of source build. Our initial solution to this was to check in the built JS assets, but this created a number of issues, including security concerns. So, we recently introduced this package to implicitly bring those files into the customer's project, and it's excluded from source build due to its NodeJS dependency. cc @javiercn, in case there's more context on the source-build problem you think should be shared. |
Will this package be shipped with the SDK, or will it be provided through nuget.org? Is this the same mechanism for the source-built and the Microsoft-built SDKs? |
I believe this package was just going to be provided on nuget.org. The intention was to make this package work in a similar way to |
The |
The test that is acquiring the package does a regular webapp. Afaik it doesn't need any Blazor assets. If a source-built SDK is expected to download this package from nuget.org, it's highly desired to not have to acquire it for applications that have no need for it. Otherwise this introduces a non source-buildable (build-time) dependency for every ASP.NET Core app built with a source-built SDK. |
We probably need to find a way to determine if Blazor will be used in AspNetCore app. Currently it's hooked by usage of Razor SDK https://github.com/dotnet/sdk/blob/main/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.FrameworkReferenceResolution.targets#L139 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you mind updating the PR description to include a summary? It would be useful to capture which tests were fixed/enabled and which remain. Also will you open tracking issues to re-enable the remain two tests once the associated product issues are resolved? TIA
Created dotnet/source-build#4835 for format test. |
@MackinnonBuck is it set up for this package to be published for the first preview(s)? |
@tmds yes, the package will get published on nuget.org for the first preview. |
Tests that are re-enabled:
Tests that are failing due to issues