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

Add Integration Test for WASM #2532

Merged
merged 14 commits into from
Mar 5, 2024
Merged

Conversation

caaavik-msft
Copy link
Contributor

@caaavik-msft caaavik-msft commented Feb 27, 2024

I'm making this PR a draft since I was not able to get it to a condition where it is good to merge in, however I think it will be a useful start for someone who is more familiar with WASM to complete it. There also would need to be some changes to the CI machines to install the wasm-tools workload and the v8 engine so that wasm can run.

This PR also includes the changes from #2531 but I think that change should go in separately and can be removed from this PR. I only include it as I needed it to test out that everything is working. I can confirm though that the issue that caused WASM runs to fail was reproducible by this integration test.

The test doesn't pass yet on my system though, but I believe it is failing because of the stuff in test-main.js which I copied from the dotnet/runtime repository and not because of any issues with BDN itself. I wasn't able to figure out how to create a minimal test-main.js to use for this integration test and just used the one from the dotnet/runtime repository, but I think maybe it relies on some v8 features or specific versions that I wasn't able to figure out. Here is the error message I get when it tries to run the js:

TypeError: dotnet.withVirtualWorkingDirectory(...).withEnvironmentVariables(...).withDiagnosticTracing(...).withExitOnUnhandledError(...).withExitCodeLogging(...).withElementOnExit(...).withInteropCleanupOnExit(...).withAssertAfterExit(...).withDumpThreadsOnNonZeroExit is not a function

@caaavik-msft caaavik-msft mentioned this pull request Feb 27, 2024
Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

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

@caaavik-msft Thank you for working on this! It would be great to have the coverage and finally stop randomly breaking WASM support.

There also would need to be some changes to the CI machines to install the wasm-tools workload and the v8 engine so that wasm can run.

We are using GH actions, so this should be doable. Here you can take a look to see how we are using npm to install some other js stuff:

- uses: actions/setup-node@v3
name: Setup node
with:
node-version: "18"
- name: Install cSpell
run: npm install -g cspell@8.0.0

Our WASM doc is outdated, but it can be helpful too: https://benchmarkdotnet.org/articles/configs/toolchains.html#wasm

tests/BenchmarkDotNet.IntegrationTests/WasmTests.cs Outdated Show resolved Hide resolved
@caaavik-msft
Copy link
Contributor Author

@radical @lewing Are one of you able to assist with writing a minimal test-main.js to be used in a WASM BDN Integration Test that is simple enough we don't have to worry about using specific versions of v8. I took the one from the runtime repo but I think it might depend on the version defined in the ChromeVersion.props file. Once we get this change in we can prevent regressions like dotnet/performance#3984 from occurring again.

@radical
Copy link
Member

radical commented Feb 28, 2024

cc @pavelsavara @maraf

@timcassell
Copy link
Collaborator

#2531 was merged, so you can rebase and remove those changes.

@pavelsavara
Copy link
Member

Are one of you able to assist with writing a minimal test-main.js

test-main.js is not great template and it also says so in the header.

I don't know what kind of setup this perf test requires, but here is simple V8 sample

Also this is public API you want to use to configure it.

@caaavik-msft caaavik-msft mentioned this pull request Mar 5, 2024
@caaavik-msft caaavik-msft marked this pull request as ready for review March 5, 2024 08:02
@caaavik-msft
Copy link
Contributor Author

@pavelsavara it seems that script is sufficient to get things working without any more customisation, thanks!

@adamsitnik @AndreyAkinshin CI is fully working now setting up everything needed for the wasm integration test to run. However, I am not sure if maybe that stuff better belongs inside the build scripts rather than in the github actions yaml. Particularly the part where we install the wasm-tools workload. Let me know what you think

Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

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

It looks great to me @caaavik-msft, big thanks for preventing future WASM breaks!

@adamsitnik
Copy link
Member

However, I am not sure if maybe that stuff better belongs inside the build scripts rather than in the github actions yaml. Particularly the part where we install the wasm-tools workload. Let me know what you think

IMO if it turns out to be a problem we can improve it later.

@AndreyAkinshin thoughts?

@caaavik-msft
Copy link
Contributor Author

Thanks to @ilonatommy who sent me the changes needed to add this functionality to the BenchmarkDotNet.Build project, I think those concerns should be addressed now and we should be ok to merge this

@adamsitnik adamsitnik merged commit 4435799 into dotnet:master Mar 5, 2024
8 checks passed
@AndreyAkinshin AndreyAkinshin added this to the v0.14.0 milestone Mar 6, 2024
@AndreyAkinshin
Copy link
Member

@caaavik-msft thank you very much for WasmTests support! Great job with extending build.cmd! I like this approach: it's consistent with the other build workflows and provides a simple way to configure the local environment. It was pretty easy to run it locally.

P.S. I have added additional docs about local setup.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider adding tests for WASM support
7 participants