[wasm][tests] Exit the script if `WasmTestRunner` build fails by radical · Pull Request #46048 · dotnet/runtime

radical · 2020-12-14T22:02:33Z

If building the WasmTestRunner.proj on helix fails, then the error
gets ignored, and script execution continues. And that results in the
failure to be shown as:

      /home/helixbot/work/9CE408BA/w/BF5C0A02/e/JIT/Methodical/eh/interactions/throw1dimarray_d/throw1dimarray_d.sh: line 136: cd: WasmApp: No such file or directory
      /home/helixbot/work/9CE408BA/w/BF5C0A02/e/JIT/Methodical/eh/interactions/throw1dimarray_d/throw1dimarray_d.sh: line 137: ./run-v8.sh: No such file or directory

.. which is misleading. Looking at the execution log shows that the real error was:

      Microsoft (R) Build Engine version 16.8.0+126527ff1 for .NET
      Copyright (C) Microsoft Corporation. All rights reserved.

        AppDir: /home/helixbot/work/9CE408BA/w/BF5C0A02/e/JIT/Methodical/eh/interactions/throw1dimarray_d/WasmApp/
        TestBinDir: /home/helixbot/work/9CE408BA/w/BF5C0A02/e/JIT/Methodical/eh/interactions/throw1dimarray_d
        ArtifactsBinDir:
      /home/helixbot/work/9CE408BA/p/build/WasmApp.targets(11,5): error : Failed to load assembly reference 'System.Windows.Forms, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null' for 'Except, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null': Could not find assembly 'System.Windows.Forms, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null'. Either explicitly load this assembly using a method such as LoadFromAssemblyPath() or use a MetadataAssemblyResolver that returns a valid assembly. [/home/helixbot/work/9CE408BA/p/wasm-test-runner/WasmTestRunner.proj]

So, exit the script if the build fails.

If building the `WasmTestRunner.proj` on helix fails, then the error gets ignored, and script execution continues. And that results in the failure to be shown as: ``` /home/helixbot/work/9CE408BA/w/BF5C0A02/e/JIT/Methodical/eh/interactions/throw1dimarray_d/throw1dimarray_d.sh: line 136: cd: WasmApp: No such file or directory /home/helixbot/work/9CE408BA/w/BF5C0A02/e/JIT/Methodical/eh/interactions/throw1dimarray_d/throw1dimarray_d.sh: line 137: ./run-v8.sh: No such file or directory ``` .. which is misleading. Looking at the execution log shows that the real error was: ``` Microsoft (R) Build Engine version 16.8.0+126527ff1 for .NET Copyright (C) Microsoft Corporation. All rights reserved. AppDir: /home/helixbot/work/9CE408BA/w/BF5C0A02/e/JIT/Methodical/eh/interactions/throw1dimarray_d/WasmApp/ TestBinDir: /home/helixbot/work/9CE408BA/w/BF5C0A02/e/JIT/Methodical/eh/interactions/throw1dimarray_d ArtifactsBinDir: /home/helixbot/work/9CE408BA/p/build/WasmApp.targets(11,5): error : Failed to load assembly reference 'System.Windows.Forms, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null' for 'Except, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null': Could not find assembly 'System.Windows.Forms, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null'. Either explicitly load this assembly using a method such as LoadFromAssemblyPath() or use a MetadataAssemblyResolver that returns a valid assembly. [/home/helixbot/work/9CE408BA/p/wasm-test-runner/WasmTestRunner.proj] ``` So, exit the script if the build fails. - And: ```diff -cd WasmApp -./run-v8.sh +cd WasmApp && ./run-v8.sh ``` Just makes it a single error, if that directory/script doesn't exist.

ghost · 2020-12-14T22:02:38Z

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

Issue Details

If building the WasmTestRunner.proj on helix fails, then the error
gets ignored, and script execution continues. And that results in the
failure to be shown as:

      /home/helixbot/work/9CE408BA/w/BF5C0A02/e/JIT/Methodical/eh/interactions/throw1dimarray_d/throw1dimarray_d.sh: line 136: cd: WasmApp: No such file or directory
      /home/helixbot/work/9CE408BA/w/BF5C0A02/e/JIT/Methodical/eh/interactions/throw1dimarray_d/throw1dimarray_d.sh: line 137: ./run-v8.sh: No such file or directory

.. which is misleading. Looking at the execution log shows that the real error was:

      Microsoft (R) Build Engine version 16.8.0+126527ff1 for .NET
      Copyright (C) Microsoft Corporation. All rights reserved.

        AppDir: /home/helixbot/work/9CE408BA/w/BF5C0A02/e/JIT/Methodical/eh/interactions/throw1dimarray_d/WasmApp/
        TestBinDir: /home/helixbot/work/9CE408BA/w/BF5C0A02/e/JIT/Methodical/eh/interactions/throw1dimarray_d
        ArtifactsBinDir:
      /home/helixbot/work/9CE408BA/p/build/WasmApp.targets(11,5): error : Failed to load assembly reference 'System.Windows.Forms, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null' for 'Except, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null': Could not find assembly 'System.Windows.Forms, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null'. Either explicitly load this assembly using a method such as LoadFromAssemblyPath() or use a MetadataAssemblyResolver that returns a valid assembly. [/home/helixbot/work/9CE408BA/p/wasm-test-runner/WasmTestRunner.proj]

So, exit the script if the build fails.

Author:	radical
Assignees:	-
Labels:	`area-Infrastructure-mono`
Milestone:	-

lewing

The changes make sense, one question about setting CLRTestExitCode when we haven't actually run the test.

src/tests/Common/CLRTest.Execute.Bash.targets

…nner-fail

Some test projects have `<CLRTestKind>RunOnly</CLRTestKind>`, eg. `GitHub_26491_SingleReturnSynchronized`. In this particular project, it doesn't have it own code/assembly, and just runs another assembly. But the default script generated for the test adds the build command for `WasmTestRunner.proj`, which fails here because there is no assembly to bundle! This manifested for `JIT/Regression/JitBlue/GitHub_26491/GitHub_26491_SingleReturnSynchronized/GitHub_26491_SingleReturnSynchronized.sh` as: ``` BEGIN EXECUTION Microsoft (R) Build Engine version 16.8.0+126527ff1 for .NET Copyright (C) Microsoft Corporation. All rights reserved. AppDir: /home/helixbot/work/AA96097C/w/9CC508B9/e/JIT/Regression/JitBlue/GitHub_26491/GitHub_26491_SingleReturnSynchronized/WasmApp/ TestBinDir: /home/helixbot/work/AA96097C/w/9CC508B9/e/JIT/Regression/JitBlue/GitHub_26491/GitHub_26491_SingleReturnSynchronized ArtifactsBinDir: /home/helixbot/work/AA96097C/p/build/WasmApp.targets(11,5): error : Could not find assembly '/home/helixbot/work/AA96097C/w/9CC508B9/e/JIT/Regression/JitBlue/GitHub_26491/GitHub_26491_SingleReturnSynchronized/GitHub_26491_SingleReturnSynchronized.dll' [/home/helixbot/work/AA96097C/p/wasm-test-runner/WasmTestRunner.proj] Test Harness Exitcode is : 1 ``` This *test* wasn't failing earlier, because the build command would fail, but the script continued execution. And the subsequent shell script worked as expected. But now we exit if the build command fails, causing the whole test to fail! Solution: Honor `$(CLRTestKind) == RunOnly`, and don't emit the build command in that case.

radical · 2020-12-15T20:02:35Z

This caused a test failure: f662177

@naricc Does the fix look correct? And/or should something be done with https://github.com/dotnet/runtime/blob/master/src/tests/JIT/Regression/JitBlue/GitHub_26491/GitHub_26491_SingleReturnSynchronized.ilproj#L7 ?

naricc · 2020-12-15T20:11:25Z

This caused a test failure: f662177

@naricc Does the fix look correct? And/or should something be done with https://github.com/dotnet/runtime/blob/master/src/tests/JIT/Regression/JitBlue/GitHub_26491/GitHub_26491_SingleReturnSynchronized.ilproj#L7 ?

Yes, I think it is the correct fix. I am not sure about why the android one was excluded that way; if it is for the same reason we should probably use the same/similar fix there. @fanyang-mono any insight?

fanyang-mono · 2020-12-15T22:07:38Z

This caused a test failure: f662177
@naricc Does the fix look correct? And/or should something be done with https://github.com/dotnet/runtime/blob/master/src/tests/JIT/Regression/JitBlue/GitHub_26491/GitHub_26491_SingleReturnSynchronized.ilproj#L7 ?

Yes, I think it is the correct fix. I am not sure about why the android one was excluded that way; if it is for the same reason we should probably use the same/similar fix there. @fanyang-mono any insight?

The reason why I excluded this test from Android, because the comment that I left there "Generated shell script has no corresponding assembly". When creating Android apps for runtime tests, I used shell script names to find the corresponding assembly. Then include that assembly to the APK. WASM doesn't have such logic. If this test used to pass when running with WASM, we should keep it that way.

fanyang-mono · 2020-12-15T22:10:20Z

src/tests/Common/CLRTest.Execute.Bash.targets

 fi

-$__Command msbuild $CORE_ROOT/wasm-test-runner/WasmTestRunner.proj /p:NetCoreAppCurrent=$(NetCoreAppCurrent) /p:TestAssembly=`pwd`/$(MsBuildProjectName).dll /p:TestBinDir=`pwd`
+$__Command msbuild $CORE_ROOT/wasm-test-runner/WasmTestRunner.proj /p:NetCoreAppCurrent=$(NetCoreAppCurrent) /p:TestAssembly=`pwd`/$(MsBuildProjectName).dll /p:TestBinDir=`pwd` || exit $?


This is how I handled the error of this script. I think it is better this way, because it covers the whole script.
https://github.com/dotnet/runtime/blob/master/src/tests/Common/CLRTest.Execute.Bash.targets#L409-L413
You could add the WASM condition there.

True, that would be better, but I wasn't sure what that might break lol. @naricc thoughts?

We can maybe merge this one, and then explore the set -e in a separate PR

I think using set -e works for android because the tests use the xharness runner which is returning 0 if the test succeeds. Wasm is using the return code the test assemblies themselves produce, which is 100 on success, but looks like an error to bash.

However, putting it as the first part of an || statement like in your PR would keep bash from seeing is as an error that requires termination with set -e.

So, regardless of whether or not we use set -e, we need this change.

Then set -e would cause it to exit early. We probably shouldn't use it for WASM.

fanyang-mono · 2020-12-15T23:37:20Z

This caused a test failure: f662177
@naricc Does the fix look correct? And/or should something be done with https://github.com/dotnet/runtime/blob/master/src/tests/JIT/Regression/JitBlue/GitHub_26491/GitHub_26491_SingleReturnSynchronized.ilproj#L7 ?

Yes, I think it is the correct fix. I am not sure about why the android one was excluded that way; if it is for the same reason we should probably use the same/similar fix there. @fanyang-mono any insight?

The reason why I excluded this test from Android, because the comment that I left there "Generated shell script has no corresponding assembly". When creating Android apps for runtime tests, I used shell script names to find the corresponding assembly. Then include that assembly to the APK. WASM doesn't have such logic. If this test used to pass when running with WASM, we should keep it that way.

Talked to Ankit offline, it seems that WASM ran into a similar issue as Android. From Ankit
"This was failing because the project doesn't have it's own assembly. It's just a wrapper project to run assembly from another project. So, trying to run WasmTestrunner for that would fail"
So we should do the same thing as Android to exclude this test from its project file.

radical · 2020-12-15T23:40:39Z

This caused a test failure: f662177
@naricc Does the fix look correct? And/or should something be done with https://github.com/dotnet/runtime/blob/master/src/tests/JIT/Regression/JitBlue/GitHub_26491/GitHub_26491_SingleReturnSynchronized.ilproj#L7 ?

Yes, I think it is the correct fix. I am not sure about why the android one was excluded that way; if it is for the same reason we should probably use the same/similar fix there. @fanyang-mono any insight?

The reason why I excluded this test from Android, because the comment that I left there "Generated shell script has no corresponding assembly". When creating Android apps for runtime tests, I used shell script names to find the corresponding assembly. Then include that assembly to the APK. WASM doesn't have such logic. If this test used to pass when running with WASM, we should keep it that way.

Talked to Ankit offline, it seems that WASM ran into a similar issue as Android. From Ankit
"This was failing because the project doesn't have it's own assembly. It's just a wrapper project to run assembly from another project. So, trying to run WasmTestrunner for that would fail"
So we should do the same thing as Android to exclude this test from its project file.

Why do we want to exclude (disable?) the test? The fix for the failure that I ran into, is to simply not build WasmTestRunner, and run the generated script, as expected by the project file, AFAIU.

fanyang-mono · 2020-12-16T00:02:45Z

This caused a test failure: f662177
@naricc Does the fix look correct? And/or should something be done with https://github.com/dotnet/runtime/blob/master/src/tests/JIT/Regression/JitBlue/GitHub_26491/GitHub_26491_SingleReturnSynchronized.ilproj#L7 ?

Yes, I think it is the correct fix. I am not sure about why the android one was excluded that way; if it is for the same reason we should probably use the same/similar fix there. @fanyang-mono any insight?

The reason why I excluded this test from Android, because the comment that I left there "Generated shell script has no corresponding assembly". When creating Android apps for runtime tests, I used shell script names to find the corresponding assembly. Then include that assembly to the APK. WASM doesn't have such logic. If this test used to pass when running with WASM, we should keep it that way.

Talked to Ankit offline, it seems that WASM ran into a similar issue as Android. From Ankit
"This was failing because the project doesn't have it's own assembly. It's just a wrapper project to run assembly from another project. So, trying to run WasmTestrunner for that would fail"
So we should do the same thing as Android to exclude this test from its project file.

Why do we want to exclude (disable?) the test? The fix for the failure that I ran into, is to simply not build WasmTestRunner, and run the generated script, as expected by the project file, AFAIU.

You are right. CLRTestKind == RunOnly prevents it from building withWasmTestrunner .

radical · 2020-12-16T00:06:45Z

Just to give this more context:

this was for src/tests/JIT/Regression/JitBlue/GitHub_2649
it has two projects:
- A: GitHub_26491_MultipleReturns.ilproj
- B: GitHub_26491_SingleReturnSynchronized.ilproj
Project A produces the test assembly. And we generate a wasm app for that.
Project B is just meant to run Project A's assembly. It does not need it's own wasm app. That's why it has
<CLRTestKind>RunOnly</CLRTestKind>
- So, as the fix, we add the build WasmTestRunner.proj command to the generated shell script only if CLRTestKind==BuildAndRun.

radical requested review from akoeplinger, lewing and naricc December 14, 2020 22:02

Dotnet-GitSync-Bot added the area-Infrastructure-mono label Dec 14, 2020

radical added the arch-wasm WebAssembly architecture label Dec 14, 2020

lewing reviewed Dec 14, 2020

View reviewed changes

src/tests/Common/CLRTest.Execute.Bash.targets Show resolved Hide resolved

naricc approved these changes Dec 14, 2020

View reviewed changes

remove change that could potentially break tests

3fdfca5

lewing approved these changes Dec 15, 2020

View reviewed changes

vargaz approved these changes Dec 15, 2020

View reviewed changes

radical added 2 commits December 15, 2020 12:54

Merge remote-tracking branch 'origin/master' into wasm-fail-on-testru…

e756074

…nner-fail

radical requested a review from naricc December 15, 2020 20:02

naricc approved these changes Dec 15, 2020

View reviewed changes

fanyang-mono reviewed Dec 15, 2020

View reviewed changes

radical mentioned this pull request Dec 15, 2020

[tests] System.Text.Json.Tests segfault, for Libraries Test Run checked coreclr Linux_musl x64 Debug #46100

Closed

radical merged commit 24a602b into dotnet:master Dec 16, 2020

radical deleted the wasm-fail-on-testrunner-fail branch December 16, 2020 00:51

ghost locked as resolved and limited conversation to collaborators Jan 15, 2021

Conversation

radical commented Dec 14, 2020

Uh oh!

ghost commented Dec 14, 2020

Uh oh!

lewing left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

radical commented Dec 15, 2020

Uh oh!

naricc commented Dec 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fanyang-mono commented Dec 15, 2020

Uh oh!

fanyang-mono Dec 15, 2020

Choose a reason for hiding this comment

Uh oh!

radical Dec 15, 2020

Choose a reason for hiding this comment

Uh oh!

radical Dec 15, 2020

Choose a reason for hiding this comment

Uh oh!

naricc Dec 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fanyang-mono Dec 16, 2020

Choose a reason for hiding this comment

Uh oh!

fanyang-mono commented Dec 15, 2020

Uh oh!

radical commented Dec 15, 2020

Uh oh!

fanyang-mono commented Dec 16, 2020

Uh oh!

radical commented Dec 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

naricc commented Dec 15, 2020 •

edited

Loading

naricc Dec 15, 2020 •

edited

Loading

radical commented Dec 16, 2020 •

edited

Loading