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 65 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

ilonatommy commented Nov 13, 2024

To be investigated - either refactoring was incorrect or using the standardized app revealed problems:
- Wasm.Build.NativeRebuild.Tests.NoopNativeRebuildTest

  • Wasm.Build.Tests.Blazor.MiscTests3 (Windows)
    - Wasm.Build.Tests.BuildPublishTests
    - Wasm.Build.Tests.Blazor.BuildPublishTests (Failed to bind to address http://127.0.0.1:5000: address already in use)
  • Wasm.Build.Tests.MT.Blazor.BlazorBuildRunTest (run fails with timeout)
  • Wasm.Build.Tests.PInvokeTableGeneratorTests: UnmanagedCallersOnly_Namespaced, EnsureComInteropCompilesInAOT
  • Wasm.Build.NativeRebuild, Wasm.Build.NativeRebuild.Tests.OptimizationFlagChangeTests (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.

Copy link
Member

@maraf maraf left a comment

Choose a reason for hiding this comment

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

I love the simplification 👏
Some semi random notes below

src/mono/wasm/Wasm.Build.Tests/Blazor/BuildPublishTests.cs Outdated Show resolved Hide resolved
bool DetectRuntimeFailures = true,
bool CheckCounter = true,
Dictionary<string, string>? ServerEnvironment = null,
Func<IPage, Task>? Test = null,
Action<IPage, IConsoleMessage>? OnConsoleMessage = null,
string? TestScenario = null,
Copy link
Member

Choose a reason for hiding this comment

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

idea: This parameter is specific to WasmBasicTestApp. I'm not sure whether to have it here. Could we do it in a way that selected TestAsset defines what parameters we have?

Copy link
Member Author

@ilonatommy ilonatommy Nov 15, 2024

Choose a reason for hiding this comment

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

This is not the only one. We also have only-blazor: CheckCounter, Test.

I see a few options:

  • Go back to 2 separate RunOptions inheriting from base class. This won't become that messy over time as having them without inheritance like we used to.
  • Fixate the scenario in the phase of creating the app - use UpdateFile on main.js in each test that uses scenarios. That shifts the responsibility to test code, to decrease the effort we can make DotnetRun, that is the most frequently used scenario, the default one.
  • Remove scenarios and convert main.js to asset in different forms. Then we will copy corresponding main.js for specific scenario and no TestScenario is needed.

src/mono/wasm/Wasm.Build.Tests/BuildProjectOptions.cs Outdated Show resolved Hide resolved
namespace Wasm.Build.Tests;

public record ProjectInfo(
string Configuration,
Copy link
Member

Choose a reason for hiding this comment

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

I think Configuration and AOT should not be here. I believe those should be just arguments for build (and possibly run). But for asset copying they are not relevant (and might be changed during test)

src/mono/wasm/Wasm.Build.Tests/ConfigSrcTests.cs Outdated Show resolved Hide resolved
new BuildProjectOptions(
info.Configuration,
info.ProjectName,
BinFrameworkDir: GetBinFrameworkDir(info.Configuration, isPublish),
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should be passing this information. It should be computed from the parameters

Copy link
Member

Choose a reason for hiding this comment

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

The same should be true for ExpectedFileType

Copy link
Member

Choose a reason for hiding this comment

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

BuildTemplateProject(info,
    new BuildProjectOptions(
        info.Configuration,
        info.ProjectName,
        BinFrameworkDir: GetBinFrameworkDir(info.Configuration, isPublish),
        ExpectedFileType: GetExpectedFileType(info, isPublish: isPublish),
        IsPublish: isPublish
));

This is a boiler plate that we have in every test. We should find a way to simplify it. I believe we should end up with BuildProject(new(info, configuration)) in most cases. If there are other parameters, like Aot, Relink, etc, just append those to the options BuildProject(new(info, configuration, Aot: true))

Copy link
Member Author

Choose a reason for hiding this comment

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

BuildOptions has ~15 fields, I would prefer to avoid converting them to method args. I agree with the other ideas, I'll go towards making it:

private _defaultBuildOptions = new BuildOptions();
BuildProject(
        ProjectInfo info,
        string configuration,
        BuildOptions buildOptions = _defaultBuildOptions))

Copy link
Member

Choose a reason for hiding this comment

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

BuildOptions as structure is fine! I'm just saying test author shouldn't be responsible for computing GetBinFrameworkDir(info.Configuration, isPublish) (none of the tests should be). Instead it should be computed by the combination of TestAsset+BuildArgs (aot, publish, config, etc)

info.ProjectName,
BinFrameworkDir: GetBinFrameworkDir(info.Configuration, isPublish),
ExpectedFileType: GetExpectedFileType(info, isPublish: isPublish),
IsPublish: isPublish,
Copy link
Member

Choose a reason for hiding this comment

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

I think publish should have separate options (maybe mostly inherited, but still)

catch (XunitException xe)
{
if (xe.Message.Contains("error CS1001: Identifier expected"))
Utils.DirectoryCopy(_projectDir!, _logPath, testOutput: _testOutput);
Copy link
Contributor

Choose a reason for hiding this comment

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

if __projectDir is non-nullable it'd avoid ! every time it's used

protected string? _projectDir
{
get => _providerOfBaseType.ProjectDir;
set => _providerOfBaseType.ProjectDir = value;
}

get => _providerOfBaseType.ProjectDir ?? throw new InvalidOperationException("_providerOfBaseType.ProjectDir cannot be null");
or
get => _providerOfBaseType.ProjectDir!;

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