-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
base: main
Are you sure you want to change the base?
Conversation
…tOfAppBundleTests`, `WasmNativeDefaultsTests`, `WasmBuildAppTest`: done
… BlazorBasicTestApp.
To be investigated - either refactoring was incorrect or using the standardized app revealed problems:
I would be happy to share this responsibility with volunteers. |
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.
I love the simplification 👏
Some semi random notes below
src/mono/wasm/Wasm.Build.Tests/BrowserStructures/BuildProduct.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, |
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.
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?
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.
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
onmain.js
in each test that uses scenarios. That shifts the responsibility to test code, to decrease the effort we can makeDotnetRun
, 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 noTestScenario
is needed.
namespace Wasm.Build.Tests; | ||
|
||
public record ProjectInfo( | ||
string Configuration, |
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.
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)
new BuildProjectOptions( | ||
info.Configuration, | ||
info.ProjectName, | ||
BinFrameworkDir: GetBinFrameworkDir(info.Configuration, isPublish), |
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.
I don't think we should be passing this information. It should be computed from the parameters
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.
The same should be true for ExpectedFileType
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.
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))
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.
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))
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.
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)
src/mono/wasm/Wasm.Build.Tests/TestAppScenarios/DownloadThenInitTests.cs
Outdated
Show resolved
Hide resolved
info.ProjectName, | ||
BinFrameworkDir: GetBinFrameworkDir(info.Configuration, isPublish), | ||
ExpectedFileType: GetExpectedFileType(info, isPublish: isPublish), | ||
IsPublish: isPublish, |
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.
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); |
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.
if __projectDir is non-nullable it'd avoid ! every time it's used
runtime/src/mono/wasm/Wasm.Build.Tests/BuildTestBase.cs
Lines 71 to 75 in 234e2f7
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!;
Main refactoring changes:
Creation of app
old - Create the information about the requested project (
BuildArgs
) and using an Action that was passed toBuildProject
, update the project that was hardcoded and used "data/test-main-7.0.js" or "test-main.js"new:
WasmBasicTestApp
orBlazorBasicTestApp
by usingCopyTestAsset
method. It is more reliable thantest-main.js
and faster than usingdotnet new
inside of the test.dotnet new {templateName}
command triggered byCreateWasmTemplateProject
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 replacementUpdateBrowserMainJs()
/UpdateBrowserProgramFile()
or custom replacements:UpdateFile()
. Big snippets of code were moved totestassets
to shorten test files. We eitherFile.Copy()
them orReplaceFile()
with them.Building the app:
old - merged with application creation step. Used
BuildProjectOptions
orBlazorBuildOptions
.new - doing only build or publish, depending on the passed
IsPublish
argument. Uses unifiedBuildProjectOptions
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.Running the app:
old - browser app using
RunAndTestWasmApp
that bases on starting aSystem.Diagnostics.Process
inRunProcessAsync
, different way of handling messages, passing arguments etc. Blazor app using Playwright inBrowserRunner
.new - both apps use Playwright (see:
BrowserRunner
) and the running logic is mostly common. They use eitherRunForBuildWithDotnetRun
orRunForPublishWithWebServer
in wasm and blazor apps.Other bigger changes:
RunHost
that supports testing with V8 on CI. We can remove it and replace withRunHost
(DotnetRun
/WebServer
), used till this point only by Blazor tests.AssertOptions
,BuildOptions
, moving records/enums to separate classes inCommon
dir etc.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).test-main.js
file works correctly:ConfigSrcTests.cs
.native-lib.o
TestAssetsPath