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

[wasm] Stop testing with V8 and wasmconsole #108711

Merged
merged 16 commits into from
Oct 16, 2024

Conversation

ilonatommy
Copy link
Member

@ilonatommy ilonatommy commented Oct 9, 2024

Follow up for #108582 (comment).

  1. Do not test v8 scenario on CI. Changelog:
| lane                           | current scenarios                               | future scenarios                   | changed? |
|--------------------------------|-------------------------------------------------|------------------------------------|----------|
| LibraryTests Linux             | WasmTestOnV8,WasmTestOnChrome,WasmTestOnFirefox | WasmTestOnChrome,WasmTestOnFirefox | Y        |
| LibraryTests Windows           | WasmTestOnChrome                                | WasmTestOnChrome                   | N        |
| LibraryTests_Smoke_AOT Linux   | WasmTestOnV8                                    | WasmTestOnChrome                   | Y        |
| LibraryTests_Smoke_AOT Windows | WasmTestOnChrome                                | WasmTestOnChrome                   | N        |
| LibraryTests_EAT Linux         | WasmTestOnV8                                    | WasmTestOnChrome                   | Y        |
| LibraryTests_Threading Linux   | WasmTestOnChrome,WasmTestOnFirefox              | WasmTestOnChrome,WasmTestOnFirefox | N        |
| extra-platforms                | WasmTestOnV8, WasmTestOnChrome                  | WasmTestOnChrome                   | Y        |
| outer-loop                     | WasmTestOnV8                                    | WasmTestOnChrome                   | Y        |
  1. Do not test wasmconsole in WasmBuildTests. Some tests were using console scenario but did not mean to target specifically console, so they had to stay but be changed to use browserwasm. This revealed a bunch of inconsistencies that required refactoring. I will try to list the biggest refactoring changes in the following points.
  2. We had WasmTemplateTestBase and WasmTemplateTestsBase that in fact could be consolidated in one class.
  3. Inheritance. We used to have, e.g.:
    NativeBuildTests <- WasmTemplateTestBase <- BlazorWasmTestBase <- WasmTemplateTestsBase <- BuildTestBase
    This was refactored to have blazor and browser apps in 2 logical "flows", e.g.:
    WorkloadRequiredTests <- BlazorWasmTestBase <- WasmTemplateTestsBase <- BuildTestBase
    NativeBuildTests <- WasmTemplateTestsBase <- BuildTestBase
  4. We used RunCommand and ToolCommand directly in the tests. Theye were exchanged for wrappers. For build/publish action we should use BuildTemplateProject and for running the browser app, RunBrowser. It is supposed to get merged with Blazor-based runners in a follow up PR, it will take too many changes to do it here.
  5. We've been doing !(buildArgs.AOT || buildArgs.Config == "Release") a lot, so it got closed in IsDotnetWasmFromRuntimePack.
  6. And a lot of building BuildProjectOptions, so we will have a "base" form of options (_basePublishProjectOptions , _baseBuildProjectOptions) that we can extend in the specific test but most frequently we use them in the default form.

The issues that were discovered when refactoring will be temporarily marked as "Issue" in the comment, then if not fixed, they will be logged as separate issues in the repo.

Follow-up PRs that will be published after this one is done:

  • Follow-up with transition to template-based tests. Instead of using [BuildAndRun( that creates a console app (with Microsoft.NET.Sdk), we should use templating mechanism. We still have ~40 tests that do not use templates.
  • Follow-up, just with refactoring. We should get rid of classes like AppTestBase, TestMainJsTestBase, TestMainJsProjectProvider and consolidate running methods from AppTestBase and WasmTemplateTestsBase, so tests derive from WasmTemplateTestsBase if they are browser apps and from BlazorWasmTestBase if they are blazor apps, nothing more. Clean up option records, BuildProjectOptions, BlazorRunOptions, BlazorBuildOptions etc.

@ilonatommy ilonatommy added arch-wasm WebAssembly architecture area-Build-mono labels Oct 9, 2024
@ilonatommy ilonatommy self-assigned this Oct 9, 2024
Copy link
Contributor

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

@ilonatommy ilonatommy marked this pull request as draft October 9, 2024 13:52
@ilonatommy ilonatommy marked this pull request as ready for review October 14, 2024 13:40
BuildTemplateProject(buildArgs, id: id, new BuildProjectOptions(
AssertAppBundle: false,
CreateProject: false,
HasV8Script: false,
Copy link
Member

Choose a reason for hiding this comment

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

V8Script could be dropped too. I expect it would many more places than just WBT. Could be in next PR

@@ -28,4 +29,14 @@ public void AssertBundle(BlazorBuildOptions options)
AssertIcuAssets: true,
AssertSymbolsFile: false // FIXME: not supported yet
));

public override string FindBinFrameworkDir(string config, bool forPublish, string framework, string? projectDir = null)
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to override it here? Blazor and Wasm SDK should behave the same

Copy link
Member Author

Choose a reason for hiding this comment

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

That's true, originally I didn't notice they are the same, it can be left in WasmSdkBasedProjectProvider only.
Note for the future: once we will align BlazorBuildOptions and AssertWasmSdkBundleOptions then maybe we will even use WASM SDK provider in Blazor and drop BlazorWasmProjectProvider.

src/mono/wasm/Wasm.Build.Tests/BuildTestBase.cs Outdated Show resolved Hide resolved

public class DebugLevelTests : AppTestBase
{
public DebugLevelTests(ITestOutputHelper output, SharedBuildPerTestClassFixture buildContext)
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 we should merge this one with Wasm.Build.Tests.DebugLevelTests and use "WasmBasicTestApp" for all of the tests.

  • Move it outside of Blazor namespace since it's not blazor specific

Copy link
Member Author

Choose a reason for hiding this comment

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

It was not done because it would require more refactoring. In the feedback commit I moved them to one file and made them use WasBasicTestApp but it broke the base class, now it's DebugLevelTests : AppTestBase. Will be cleaned up better in a follow-up

Publish: true,
TargetFramework: BuildTestBase.DefaultTargetFramework,
UseCache: false,
IsBrowserProject: isBrowser,
IsBrowserProject: true,
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 we can remove IsBrowserProject or similar if we have any

Copy link
Member Author

Choose a reason for hiding this comment

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

Not yet. We have a significant bundle assert check that based on its value uses either the wasm sdk-like way assert or deprecated but still used by tests (to be removed in a follow up) mainJs-like assert. This would lead to too many other changes. Later

Copy link
Member

Choose a reason for hiding this comment

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

Since everything is now Wasm SDK, I think we can remove this base class

Copy link
Member

Choose a reason for hiding this comment

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

Am I right? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it can be removed in the follow up that will be getting rid of TestMainJsProjectProvider that is still based on ProjectProviderBase and used in ~16 test files.

@ilonatommy ilonatommy merged commit 0290065 into dotnet:main Oct 16, 2024
154 of 158 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Nov 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly architecture area-Build-mono
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants