Skip to content
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

All WasmBuildTests use dotnet new, unification #109069

Open
wants to merge 56 commits into
base: main
Choose a base branch
from

Conversation

ilonatommy
Copy link
Member

@ilonatommy ilonatommy commented Oct 21, 2024

Main refactoring changes:

Creation of app

old - Create the information about the requested project (BuildArgs) and using an Action that was passed to BuildProject, update the project that was hardcoded and used "data/test-main-7.0.js" or "test-main.js"

InitProject: () => File.WriteAllText(Path.Combine(_projectDir!, "Program.cs"), s_mainReturns42),

new:

  • Copy an application prepared for testing multiple scenarios, WasmBasicTestApp or BlazorBasicTestApp by using CopyTestAsset method. It is more reliable than test-main.js and faster than using dotnet new inside of the test.
  • Use dotnet new {templateName} command triggered by CreateWasmTemplateProject and return object that contains information about the created project. All the editions to the template are explicitly done in the test by replacement methods, either default replacement UpdateBrowserMainJs() / UpdateBrowserProgramFile() or custom replacements: UpdateFile(). Big snippets of code were moved to testassets to shorten test files. We either File.Copy() them or ReplaceFile() with them.
ProjectInfo info = CopyTestAsset(config, aot, "WasmBasicTestApp", prefix, "App", extraProperties: extraProperties);
ProjectInfo info = CreateWasmTemplateProject(Template.WasmBrowser, config, aot, prefix, extraProperties: extraProperties);

Building the app:

old - merged with application creation step. Used BuildProjectOptions or BlazorBuildOptions.

 BuildProject(buildArgs,
                            id: id,
                            new BuildProjectOptions(
                                InitProject: () => File.WriteAllText(Path.Combine(_projectDir!, "Program.cs"), programText),
                                DotnetWasmFromRuntimePack: dotnetWasmFromRuntimePack,
                                GlobalizationMode: GlobalizationMode.Hybrid));

new - doing only build or publish, depending on the passed IsPublish argument. Uses unified BuildProjectOptions for browser app and blazor app. GetExpectedFileType is unified for all the tests - we avoid the need to check the condition for FromRuntimePack/AOT/Relinked dotnet in each test.

BuildTemplateProject(info,
                        new BuildProjectOptions(
                            config,
                            info.ProjectName,
                            BinFrameworkDir: GetBinFrameworkDir(config, isPublish),
                            ExpectedFileType: GetExpectedFileType(info, isPublish: isPublish, isNativeBuild: isNativeBuild),
                            IsPublish: isPublish,
                            GlobalizationMode: GlobalizationMode.Hybrid
                        ));

Running the app:

old - browser app using RunAndTestWasmApp that bases on starting a System.Diagnostics.Process in RunProcessAsync, different way of handling messages, passing arguments etc. Blazor app using Playwright in BrowserRunner.

new - both apps use Playwright (see: BrowserRunner) and the running logic is mostly common. They use either RunForBuildWithDotnetRun or RunForPublishWithWebServer in wasm and blazor apps.

Other bigger changes:

  • Avoid testing AOT + Debug unless we're checking if it fails properly - it got blocked in [wasm] Disallow not useful configuration combinations #104149 and since on CI we typically skip debug, there is a lot of test that fail when run locally but it's not a bug.
  • Tests that use old way of project creation use also old RunHost that supports testing with V8 on CI. We can remove it and replace with RunHost (DotnetRun / WebServer), used till this point only by Blazor tests.
  • Unification of records - AssertOptions, BuildOptions, moving records/enums to separate classes in Common dir etc.
  • Removal of:
  • BlazorWasmProjectProvider - when we have only bowser-sdk-based tests, we don't need multiple providers. The base provider will be removed as well.
  • TestMainJsProjectProvider - same as above, it was for wasm console tests.
  • TestMainJsTestBase - removal + edition of all the tests that were using it (16 files).
  • Blazor tests if they are not Blazor-specific, e.g. ICU tests check behavior of browser sdk code that is shared for both apps.
  • Tests that were testing if test-main.js file works correctly: ConfigSrcTests.cs.
  • Adding "TestOutput ->" to strings we want to check in the test forces recompilation of native lib native-lib.o
  • Moving constant code snippets to TestAssetsPath

@ilonatommy ilonatommy added arch-wasm WebAssembly architecture test-enhancement Improvements of test source code area-Build-mono labels Oct 21, 2024
@ilonatommy ilonatommy self-assigned this Oct 21, 2024
@ilonatommy
Copy link
Member Author

To be investigated - either refactoring was incorrect or using the standardized app revealed problems:

  • Wasm.Build.NativeRebuild.Tests.NoopNativeRebuildTest
  • Wasm.Build.NativeRebuild.Tests.OptimizationFlagChangeTests
  • Wasm.Build.Tests.Blazor.MiscTests3 (Windows)
  • Wasm.Build.Tests.BuildPublishTests
  • Wasm.Build.Tests.MT.Blazor.BlazorBuildRunTest (run fails with timeout)
  • Wasm.Build.Tests.PInvokeTableGeneratorTests: UnmanagedCallersOnly_Namespaced, EnsureComInteropCompilesInAOT
  • Wasm.Build.NativeRebuild (file sizes don't match)
  • Wasm.Build.Tests.MainWithArgsTests (passing args to the program does not work)
  • Wasm.Build.Tests.HybridGlobalizationTests (dotnet.globalization.js not found) - HG is going to be mostly off, so not high priority
  • Wasm.Build.Tests.WasmBuildAppTest (from some reason we cannot read from the config file)

I would be happy to share this responsibility with volunteers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arch-wasm WebAssembly architecture area-Build-mono test-enhancement Improvements of test source code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants