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] Wasm.Build.Tests: add support for shared builds #49398

Merged
merged 24 commits into from
Mar 29, 2021

Conversation

radical
Copy link
Member

@radical radical commented Mar 9, 2021

  • Essentially, we want to share builds wherever possible. Example cases:

    • Same build, but run with different hosts like v8/chrome/safari, as
      separate test runs
    • Same build but run with different command line arguments
  • Sharing builds especially helps when we are AOT'ing, which is slow!

  • This is done by caching the builds with the key:

    public record BuildArgs(string ProjectName, string Config, bool AOT, string ProjectFileContents, string? ExtraBuildArgs);

  • Also, SharedBuildClassFixture is added, so that the builds can be
    cleaned up after all the tests in a particular class have finished
    running.

  • Each test run gets a randomly generated test id. This is used for
    creating:

    1. build paths, like artifacts/bin/Wasm.Build.Tests/net6.0-Release/browser-wasm/xharness-output/logs/n1xwbqxi.ict
    2. and the log for running with xharness, eg. for Chrome, are in
      artifacts/bin/Wasm.Build.Tests/net6.0-Release/browser-wasm/xharness-output/logs/n1xwbqxi.ict/Chrome/
  • split WasmBuildAppTest.cs into : BuildTestBase.cs, and
    MainWithArgsTests.cs.

Note: In interest of lowering test run times, this builds the tests in Release config only, on CI.
But when running the tests locally, both Debug, and Release configs are used.

@ghost
Copy link

ghost commented Mar 9, 2021

Tagging subscribers to this area: @directhex
See info in area-owners.md if you want to be subscribed.

Issue Details
  • Essentially, we want to share builds wherever possible. Example cases:

    • Same build, but run with different hosts like v8/chrome/safari, as
      separate test runs
    • Same build but run with different command line arguments
  • Sharing builds especially helps when we are AOT'ing, which is slow!

  • This is done by caching the builds with the key:

    public record BuildArgs(string ProjectName, string Config, bool AOT, string ProjectFileContents, string? ExtraBuildArgs);

  • Also, SharedBuildClassFixture is added, so that the builds can be
    cleaned up after all the tests in a particular class have finished
    running.

  • Each test run gets a randomly generated test id. This is used for
    creating:

    1. build paths, like artifacts/bin/Wasm.Build.Tests/net6.0-Release/browser-wasm/xharness-output/logs/n1xwbqxi.ict
    2. and the log for running with xharness, eg. for Chrome, are in
      artifacts/bin/Wasm.Build.Tests/net6.0-Release/browser-wasm/xharness-output/logs/n1xwbqxi.ict/Chrome/
  • split WasmBuildAppTest.cs into : BuildTestBase.cs, and
    MainWithArgsTests.cs.

Author: radical
Assignees: -
Labels:

area-Infrastructure-mono

Milestone: -

@radical radical marked this pull request as draft March 9, 2021 23:51
@radical radical added the arch-wasm WebAssembly architecture label Mar 9, 2021
@ghost
Copy link

ghost commented Mar 9, 2021

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

Issue Details
  • Essentially, we want to share builds wherever possible. Example cases:

    • Same build, but run with different hosts like v8/chrome/safari, as
      separate test runs
    • Same build but run with different command line arguments
  • Sharing builds especially helps when we are AOT'ing, which is slow!

  • This is done by caching the builds with the key:

    public record BuildArgs(string ProjectName, string Config, bool AOT, string ProjectFileContents, string? ExtraBuildArgs);

  • Also, SharedBuildClassFixture is added, so that the builds can be
    cleaned up after all the tests in a particular class have finished
    running.

  • Each test run gets a randomly generated test id. This is used for
    creating:

    1. build paths, like artifacts/bin/Wasm.Build.Tests/net6.0-Release/browser-wasm/xharness-output/logs/n1xwbqxi.ict
    2. and the log for running with xharness, eg. for Chrome, are in
      artifacts/bin/Wasm.Build.Tests/net6.0-Release/browser-wasm/xharness-output/logs/n1xwbqxi.ict/Chrome/
  • split WasmBuildAppTest.cs into : BuildTestBase.cs, and
    MainWithArgsTests.cs.

Author: radical
Assignees: -
Labels:

arch-wasm, area-Infrastructure-mono

Milestone: -

@radical radical force-pushed the shared-wasm-build-tests branch 3 times, most recently from 3f95a20 to 78840c3 Compare March 10, 2021 21:52
@radical radical marked this pull request as ready for review March 10, 2021 22:43
@radical
Copy link
Member Author

radical commented Mar 11, 2021

This will be rebased once #49446 is merged.

- Essentially, we want to share builds wherever possible. Example cases:

    - Same build, but run with different hosts like v8/chrome/safari, as
      separate test runs
    - Same build but run with different command line arguments

- Sharing builds especially helps when we are AOT'ing, which is slow!

- This is done by caching the builds with the key:

    `public record BuildArgs(string ProjectName, string Config, bool AOT, string ProjectFileContents, string? ExtraBuildArgs);`

- Also, ` SharedBuildClassFixture` is added, so that the builds can be
  cleaned up after all the tests in a particular class have finished
  running.

- Each test run gets a randomly generated test id. This is used for
  creating:
  1. build paths, like `artifacts/bin/Wasm.Build.Tests/net6.0-Release/browser-wasm/xharness-output/logs/n1xwbqxi.ict`
  2. and the log for running with xharness, eg. for Chrome, are in
     `artifacts/bin/Wasm.Build.Tests/net6.0-Release/browser-wasm/xharness-output/logs/n1xwbqxi.ict/Chrome/`

- split `WasmBuildAppTest.cs` into : `BuildTestBase.cs`, and
  `MainWithArgsTests.cs`.
For AOT we generate `pinvoke-table.h` in the obj directory. But there is
one present in the runtime pack too.

In my earlier changes the order in which these were passed as include
search paths was changed from:

`"-I/runtime/pack/microsoft.netcore.app.runtime.browser-wasm/Release/runtimes/browser-wasm/native/include/wasm" "-Iartifacts/obj/mono/Wasm.Console.Sample/wasm/Release/browser-wasm/wasm/"`

.. which meant that the one from the runtime pack took precedence, and
got used. So, fix the order!

And change the property names to indicate where they are sourced from.
The environment variable is set on helix. During local testing it can be
useful when using a locally built xharness.
@radical
Copy link
Member Author

radical commented Mar 12, 2021

Unrelated test failures.

Copy link
Member

@steveisok steveisok left a comment

Choose a reason for hiding this comment

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

LGTM. I'm curious the average helix run time w/ these changes.

@radical
Copy link
Member Author

radical commented Mar 16, 2021

AFAICS, the test time stays at about 45mins. With this one we actually are building only Release, but #49559 will enable testing for Debug too. And I will add support to enable more parallelization.

Though when I look at the run times for the individual runs for wasm.build tests, they seem to be around ~25mins.

@radical
Copy link
Member Author

radical commented Mar 16, 2021

AFAICS, the test time stays at about 45mins.

The helix step runs in total 45mins when I looked at one of the earlier main builds. The wasm.build test run itself takes ~25mins, it runs in parallel to the library tests.

@radical
Copy link
Member Author

radical commented Mar 16, 2021

Repeatedly getting test run failures:

.packages/microsoft.dotnet.helix.sdk/6.0.0-beta.21155.1/tools/Microsoft.DotNet.Helix.Sdk.MonoQueue.targets(47,5): error : (NETCORE_ENGINEERING_TELEMETRY=Helix) RestApiException`1: The response contained an invalid status code 500 Internal Server Error

Body: {"Message":"An error occured.","ActivityId":"0ade13a01812c1468a790d153e2fc3a0"}
   at Microsoft.DotNet.Helix.Client.Storage.OnNewFailed(Request req, Response res) in /_/src/Microsoft.DotNet.Helix/Client/CSharp/generated-code/Storage.cs:line 202
   at Microsoft.DotNet.Helix.Client.Storage.NewAsync(ContainerCreationRequest body, CancellationToken cancellationToken) in /_/src/Microsoft.DotNet.Helix/Client/CSharp/generated-code/Storage.cs:line 164
   at Microsoft.DotNet.Helix.Client.ApiBlobHelper.GetContainerAsync(String requestedName, String targetQueue, CancellationToken cancellationToken) in /_/src/Microsoft.DotNet.Helix/JobSender/StorageHelpers/ApiBlobHelper.cs:line 22
   at Microsoft.DotNet.Helix.Client.JobDefinition.SendAsync(Action`1 log, CancellationToken cancellationToken) in /_/src/Microsoft.DotNet.Helix/JobSender/JobDefinition.cs:line 157
   at Microsoft.DotNet.Helix.Sdk.SendHelixJob.ExecuteCore(CancellationToken cancellationToken) in /_/src/Microsoft.Dot

.. tracked in https://github.com/dotnet/core-eng/issues/12499 .

@radical
Copy link
Member Author

radical commented Mar 17, 2021

Added a few more tests, and some cleanup.

@radical
Copy link
Member Author

radical commented Mar 26, 2021

investigating the test run time

TLDR;
- this might help with the job getting scheduled first, and thus having
  a chance at completing at the same time as others.

Reasoning:

The problem this is trying to fix is:

1. The helix step submits 3 jobs:
    a. library tests to be run with v8
    b. library tests to be run with browser (scenario=wasmtestonbrowser)
    c. Wasm.Build.Tests (scenario=buildwasmapps)

2. The 3 jobs, individually take roughly 30mins each
3. And they get submitted at roughly the same time
4. But .. the first two seem to complete earlier, and the 3rd one
   completes 25-30mins later.

The hypothesis is that all the machines might be busy processing the
200+ work items from each of the first two steps, and so
Wasm.Build.Tests get scheduled pretty late.

So, here we move that to be submitted first, in the hope that it would
be able to run in parallel with the other jobs.
@radical
Copy link
Member Author

radical commented Mar 26, 2021

@akoeplinger with your suggested change (7caed5a) it ran in 43mins, instead of 59! 👍

@radical
Copy link
Member Author

radical commented Mar 26, 2021

@lewing @steveisok this is ready for another look! And hopefully merge :D

@radical radical merged commit 617a18d into dotnet:main Mar 29, 2021
@radical radical deleted the shared-wasm-build-tests branch March 29, 2021 19:33
@ghost ghost locked as resolved and limited conversation to collaborators Apr 28, 2021
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants