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 Wasm testing in Chromium, not just command-line v8 #40786

Merged
merged 43 commits into from
Sep 29, 2020

Conversation

directhex
Copy link
Member

This should give us a better idea of any cases where browser and JS engine behaviour diverge.

@Dotnet-GitSync-Bot
Copy link
Collaborator

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@ghost
Copy link

ghost commented Aug 13, 2020

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

@directhex
Copy link
Member Author

Failures are because https://dnceng.visualstudio.com/internal/_git/dotnet-helix-machines/pullrequest/9677 hasn't landed in prod Helix instance yet.

@@ -334,6 +334,11 @@ jobs:
extraStepsParameters:
creator: dotnet-bot
testRunNamePrefixSuffix: Mono_$(_BuildConfig)
scenarios:
# Only one scenario per build for jobs which install xharness CLI
# until we fix https://github.com/dotnet/arcade/issues/5919
Copy link
Member

Choose a reason for hiding this comment

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

dotnet/arcade#5919 should be fixed now, but you might need to update the Helix SDK since Arcade wasn't released yet

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried bumping Arcade as part of this for testing and EVERYTHING EXPLODED so I'll wait for a bump to hit master I think

Copy link
Member

Choose a reason for hiding this comment

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

@directhex bump Helix SDK, not arcade. Check the iOS draft PR and cherry pick that

@@ -14,7 +14,7 @@

<PropertyGroup Condition="'$(TargetOS)' == 'Browser'">
<!-- We need to set this in order to get extensibility on xunit category traits and other arguments we pass down to xunit via MSBuild properties -->
<RunScriptCommand>$HARNESS_RUNNER wasm test --engine=$(JSEngine) $(JSEngineArgs) --js-file=runtime.js -v --output-directory=$XHARNESS_OUT -- --run WasmTestRunner.dll $(AssemblyName).dll</RunScriptCommand>
<RunScriptCommand>$HARNESS_RUNNER wasm test$TESTINBROWSER --app=. --engine=$(JSEngine) $(JSEngineArgs) --js-file=runtime.js -v --output-directory=$XHARNESS_OUT -- --run WasmTestRunner.dll $(AssemblyName).dll</RunScriptCommand>
Copy link
Member

Choose a reason for hiding this comment

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

nit: I'd prefer substituting the whole command rather than appending only the -browser portion.

E.g.

Suggested change
<RunScriptCommand>$HARNESS_RUNNER wasm test$TESTINBROWSER --app=. --engine=$(JSEngine) $(JSEngineArgs) --js-file=runtime.js -v --output-directory=$XHARNESS_OUT -- --run WasmTestRunner.dll $(AssemblyName).dll</RunScriptCommand>
<RunScriptCommand>$HARNESS_RUNNER wasm $XHARNESS_COMMAND --app=. --engine=$(JSEngine) $(JSEngineArgs) --js-file=runtime.js -v --output-directory=$XHARNESS_OUT -- --run WasmTestRunner.dll $(AssemblyName).dll</RunScriptCommand>

and then setting XHARNESS_COMMAND to test or test-browser depending on Scenario.

@steveisok
Copy link
Member

Definitely looks like we're still blocked by #41160 and #41269.

.. on X509 certificates.
Tests discovery fails on
`System.Net.Http.Functional.Tests.HttpClientEKUTest..cctor()` which ends
up throwing PNSE for

`Certificates cctor threw System.PlatformNotSupportedException: System.Security.Cryptography.X509Certificates is not supported on this platform.`

Because it's a static ctor, `SkipOnMono` will not be helpful to skip
this one. So, we use `#if TARGETS_BROWSER` instead.
@radical
Copy link
Member

radical commented Sep 25, 2020

Should I open a separate PR for fixing System.Net.Http/tests/FunctionalTests? Or add it here? https://gist.github.com/radical/f7f1a99ac9f4bd33554056401fdfeb4f

@steveisok
Copy link
Member

I'd probably just add it here.

Steve Pfister and others added 6 commits September 25, 2020 18:05
- This showed up when running System.Net.Http's FunctionalTests, as
creating a listening socket fails there with

`System.PlatformNotSupportedException : System.Net.Sockets is not supported on this platform.`

- Also, surface the exception so it can correctly fail tests.
System.Net.Http.Functional.Tests.SyncHttpHandler_HttpClient.HandlerTest.GetAsync_InvalidUrl_ExpectedExceptionThrown
@radical
Copy link
Member

radical commented Sep 26, 2020

System.Net.Http's FunctionalTests can now be enabled by changing SkipOnMono in src/libraries/System.Net.Http/tests/FunctionalTests/AssemblyInfo.cs.

And dotnet/xharness#313 - has some nice additions.


if (is_browser) {
// We expect to be run by tests/runtime/run.js which passes in the arguments using http parameters
window.real_print = console.log;
Copy link
Member

@lewing lewing Sep 27, 2020

Choose a reason for hiding this comment

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

I want this in asap but after it lands I think we should consider sending all this as json over the websocket and expand it on the xharness side, we should be able to clean up the console output considerably.

pseudo code like

JSON.stringify ({ 'method': method, args: {...args}}) 

where method is { 'print', 'console.debug', ... }

and after that it might make sense to add a custom log provider to preserve even more structure.


consoleWebSocket = new WebSocket(consoleUrl);
consoleWebSocket.onopen = function(event) {
consoleWebSocket.send("browser: Console websocket connected.");
Copy link
Member

Choose a reason for hiding this comment

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

This might be a better place to force everything through print? I sometimes use runtime-tests.js outside of xharness

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that makes sense. And I was thinking maybe the WebSocket thing should be optional? I found it useful to run the tests without the harness, directly in a browser.

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 it is worth a follow-up once we have everything working in CI.

Update from channel - .NET Eng - Latest

Updating 'Microsoft.DotNet.XHarness.CLI': '1.0.0-prerelease.20474.1' => '1.0.0-prerelease.20476.2' (from build '20200926.2' of 'https://github.com/dotnet/xharness')
Updating 'Microsoft.DotNet.XHarness.TestRunners.Xunit': '1.0.0-prerelease.20474.1' => '1.0.0-prerelease.20476.2' (from build '20200926.2' of 'https://github.com/dotnet/xharness')
@radical
Copy link
Member

radical commented Sep 27, 2020

1 failing test remaining:

      [FAIL] System.Xml.Tests.AsyncReaderLateInitTests.ReadAsyncAfterInitializationWithUriThrows

      System.InvalidOperationException : An asynchronous operation is already in progress.     
         at System.Xml.XmlAsyncCheckReader.CheckAsync()
         at System.Xml.XmlAsyncCheckReader.Dispose(Boolean disposing)
         at System.Xml.XmlReader.Dispose()
         at System.Xml.Tests.AsyncReaderLateInitTests.ReadAsyncAfterInitializationWithUriThrows()
         at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)

@steveisok
Copy link
Member

@radical I'm ok with skipping the last failing test.

@akoeplinger Can you give this a final review to move things forward?

@steveisok steveisok self-requested a review September 28, 2020 22:09
@lewing
Copy link
Member

lewing commented Sep 28, 2020

@steveisok can we review System.Xml.Tests.AsyncReaderLateInitTests the for the actual browser case after this.

@@ -941,6 +943,7 @@ public sealed class SocketsHttpHandler_SchSendAuxRecordHttpTest : SchSendAuxReco
public SocketsHttpHandler_SchSendAuxRecordHttpTest(ITestOutputHelper output) : base(output) { }
}

[SkipOnMono("Tests hang with chrome. To be investigated", TestPlatforms.Browser)]
Copy link
Member

Choose a reason for hiding this comment

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

do we have a tracking issue for this?

Copy link
Member

Choose a reason for hiding this comment

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

Should any of the SocketsHttpHandler tests be working?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, SocketsHttpHandler should throw PNSE on wasm-browser

Copy link
Member

Choose a reason for hiding this comment

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

Ok, let's follow up on this.

@lewing
Copy link
Member

lewing commented Sep 29, 2020

Opened #42852 to discuss the loopback server problem

@akoeplinger akoeplinger merged commit b25c7c4 into dotnet:master Sep 29, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants