-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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] Wasm.Build.Tests - some refactoring, and rationalizing #88357
Conversation
.. so we can transparently get console output if needed.
Add more centralized ways to check for `dotnet*` files in the output. Allow these to differ between test-main.js, blazor, and wasm templates. This tries to only add new code, and skips moving the logical test code from BuildTestBase.cs to the various providers. That will done in a follow up PR.
Tagging subscribers to 'arch-wasm': @lewing Issue Details
|
I recommend looking at this commit wise, which should make reviewing a little easier. |
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.
Looks very good
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.
It is a good improvement! I have few comments/ideas.
: ProjectProviderBase(projectDir, _testOutput) | ||
{ | ||
public void AssertBlazorBootJson(string config, bool isPublish, bool isNet7AndBelow, string targetFramework, string? binFrameworkDir) | ||
{ |
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.
Blazor boot json schema is now shared with WasmAppBuilder. We should do the same check for every app bundle
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.
Yep, that will in the follow up PRs. I'm trying to keep the changes to a minimum, like here the essential change is the check for dotnet files. The next one will add some new test classes and mostly move code around. And the one after that can then cleanly move the templates to wasm-sdk. Followed up multithreaded tests.
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.
And the same thing is needed for checking icu files.
{ | ||
// no fingerprinting | ||
protected override IReadOnlyDictionary<string, bool> GetDotnetFilesKnownSet(RuntimeVariant runtimeType) | ||
=> new SortedDictionary<string, bool>() |
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.
Could we merge GetDotnetFilesKnownSet
and GetDotNetFilesExpectedSet
and provide a collection of records (with properties)?
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.
And maybe a second thought. If I image that I add a new runtime file, I would need to find all places and add the file there. What I like about similar SDK tests, is that in such a case, I can just run all tests in mode "generate baselines" and than check the generated changes in git diff.
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 baselines stuff has been a major pain in sdk tests, and it's already well covered there. So I don't think we need to duplicate that here. One of the reasons for moving to this new way of testing for dotnet files was so the logic is all shared, and the difference is just the list of files. And the reason for difference list is that we still have different kinds of builds - blazor, templates, and test-main-js based tests, each having differences in their bundles. And these will gradually converge.
What I can do is try to ensure that a new runtime file would have to be added in only these lists. Does that make sense?
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 want to duplicate baseline tests from SDK, I'm just looking for a good way to discover all places for scenario when a new runtime file is added.
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.
Could we merge GetDotnetFilesKnownSet and GetDotNetFilesExpectedSet and provide a collection of records (with properties)?
Do we need to method with two lists of files? The lists look identical. What am I missing?
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.
One is the list of all the known dotnet.*
files to expect for a given project type, and whether to expect them to be fingerprinted. The other is for the actual list expected for individual tests. I can look at possibly improving this in the follow up PR.
This adds new
*ProjectProvider
classes that will have methods for project-type specific things like expected list ofdotnet.*
files, methods to create/run projects. This is the main new change here. A streamlined way of knowing which files we are looking for in tests.isNet7AndBelow
which was unused. But in a follow up PR, blazornet7.0
project will be addedBuildTestBase.cs
has lot of project-type specific code, but almost none of that has been moved to the provider classes, in an effort to keep this PR small.Options
classes.multithreaded
projects, but it isn't used as yet, but will get utilized in a follow up PR.