CoreFX CI Unix #18753

A-And · 2018-07-02T20:39:40Z

Unix Version of #18365
The work left to do on this is to add currently failing tests.

Add msg prefix Change if condition

jkotas · 2018-07-02T21:01:19Z

tests/CoreFX/TopN.CoreFX.x64.Unix.issues.json

+            "methods": [
+                {
+                    "name": "System.Reflection.Context.Tests.CustomPropertyInfoTests.GetCustomAttributesDataTest",
+                    "reason": "System.PlatformNotSupportedException System.PlatformNotSupportedException : Customized reflection contexts are only supported on .NET Framework."


This exception means that we are using wrong corefx bits (.NET Framework bits instead of netcoreapp30 bits) when running the tests. It would be nice to fix the problem with the wrong corefx bits - there seems to be a lot of the test exclusion because of this underlying problem.

This should fix the failures here - #18760 .

jashook · 2018-07-02T22:50:59Z

These changes conflict with #18695 @sbomer heads up.

A-And · 2018-07-02T23:03:34Z

@dotnet-bot test OSX10.12 x64 Checked Innerloop Build and Test please

BruceForstall · 2018-07-03T18:21:25Z

build-test.sh

    generate_layout
+    if [ "$__GenerateTestHostOnly" ]; then
+        echo "Generating test host..."        
+        generate_testhost


I don't understand this: generate_layout already will have invoked generate_testhost. For generatetesthostonly is it required to also first build the layout? If building the layout also generates the testhost, why do we even need the generatetesthostonly option?

I'm no bash expert, but should if [ "$__GenerateTestHostOnly" ]; be if [ -n "$__GenerateTestHostOnly" ];?

Great catch - thank you! The generate_testhost in generate_layout was unnecessary.
The test host (i.e. the CoreFX testing framework) has a dependency on generating the test layout (which creates CORE_ROOT) I've kept the generatetesthostonly in order to be able to generate just CORE_ROOT if necessary.

BruceForstall · 2018-07-03T18:23:41Z

tests/runtest.proj

    </PropertyGroup>

    <PropertyGroup Condition="'$(OSGroup)'!='Windows_NT'">
      <HostFxrFileName>libhostfxr</HostFxrFileName>
      <HostFxrFileExtension Condition="'$(OSGroup)' == 'Linux' Or '$(OSGroup)' == 'FreeBSD'">so</HostFxrFileExtension>
      <HostFxrFileExtension Condition="$(OSGroup) =='OSX'">dylib</HostFxrFileExtension>
+      <HostPolicyFileName>libhostpolicy</HostPolicyFileName>
+      <HostPolicyExtension Condition="'$(OSGroup)' == 'Linux' Or '$(OSGroup)' == 'FreeBSD'">so</HostPolicyExtension>
+      <HostPolicyExtension Condition="$(OSGroup) =='OSX'">dylib</HostPolicyExtension>


I don't know the syntax here, but instead of duplicating logic, couldn't it just copy the HostFxrFileExtension value? Something like:

<HostPolicyExtension>$(HostFxrFileExtension)</HostPolicyExtension>

BruceForstall · 2018-07-03T18:25:18Z

tests/runtest.sh

+    echo 'CoreFX Test Options '
+    echo '  --corefxtests                    : Runs CoreFX tests'
+    echo '  --corefxtestsall                 : Runs all available CoreFX tests'
+    echo '  --corefxtestlist                 : Runs the CoreFX tests specified in the passed list'   


The --corefxtestlist option should show how the "list" is passed. E.g., --corefxtestlist=<list>?

BruceForstall · 2018-07-03T18:28:21Z

tests/runtest.sh

+            local coreFXTestRemoteURL=$(<${coreClrSrcTestDir}/CoreFX/CoreFXTestListURL_OSX.txt)
+            local coreFXTestExclusionDef=nonosxtests
+        ;;        
+        # Default to Linux        


If the default is Linux, maybe you can just delete the (duplicative) Linux case, above.

BruceForstall · 2018-07-03T21:31:54Z

If you plan to merge this, please remove the [WIP] tag from the issue title.

jashook

Lgtm minus some nits. Still concerned about the merge between this and #18695

jashook · 2018-07-03T21:59:50Z

tests/runtest.sh

+
+    if [ -z $coreClrBinDir ]; then
+        local coreClrBinDir=${coreClrSrc}/bin
+        export __CoreFXTestDir=${coreClrSrc}/bin/tests/CoreFX


Why do we have two possible locations for this?

Reducing verbosity. Instead of running runtest.sh with both --coreclr-src and --coreclr-bin we infer the bin directory.
If someone were to supply both it would change the output directory of the helper projects and logs.

jashook · 2018-07-03T22:02:18Z

tests/runtest.sh

+    local coreFXLogDir=${coreClrBinDir}/Logs/CoreFX/
+    local coreFXTestExecutableArgs="--notrait category=nonnetcoreapptests --notrait category=${coreFXTestExclusionDef} --notrait category=failing --notrait category=IgnoreForCI --notrait category=OuterLoop --notrait Benchmark=true"
+
+    # What happens on distros where msbuild is not supported?


Please remove this comment, unsupported targets should get a warning earlier in the file.

You're right - this will be caught (or overriden) in build-test before this is even called.

jashook · 2018-07-03T22:05:12Z

tests/runtest.sh

+    buildCommand="${dotnetExe} msbuild ${coreFXTestSetupUtility} /p:OutputPath=${coreFXTestSetupUtilityOutputPath} /p:Platform=${_arch} /p:Configuration=Release"
+    echo $buildCommand    
+    # Invoke MSBuild
+    eval $buildCommand


I am not a fan of the eval $buildCommand, I would prefer to invoke the command and pass the extra args as a variable.

Something like:

"${dotnetExe}" $buildArgs

jashook · 2018-07-03T22:05:50Z

tests/runtest.sh

+    # Invoke MSBuild
+    eval $buildCommand
+
+    chmod +x $dotnetExe


Should this be before the eval?

jashook · 2018-07-03T22:06:11Z

tests/runtest.sh

+
+    runCommand="${dotnetExe} ${coreFXTestSetupUtilityOutputPath}/${coreFXTestSetupUtilityName}.dll --clean --outputDirectory ${coreFXTestBinariesOutputPath} --testListJsonPath ${CoreFXTestList} ${coreFXRunCommand} --dotnetPath ${testHostDir}/dotnet --testUrl ${coreFXTestRemoteURL} --executable ${coreFXTestExecutable} --log ${coreFXLogDir} ${coreFXTestExecutableArgs}"
+    echo $runCommand
+    eval $runCommand


See previous comment about eval

A-And · 2018-07-09T20:00:24Z

@dotnet-bot test this please

A-And · 2018-07-10T20:04:50Z

@dotnet-bot test OSX10.12 x64 Checked CoreFX Tests please

A-And · 2018-07-11T16:50:36Z

@dotnet-bot test OSX10.12 x64 Checked Innerloop Build and Test please
@dotnet-bot test OSX10.12 x64 Checked CoreFX Tests please

A-And · 2018-07-12T00:26:45Z

@dotnet-bot test OSX10.12 x64 Checked Innerloop Build and Test please

A-And · 2018-07-12T00:42:09Z

@dotnet-bot test OSX10.12 x64 Checked CoreFX Tests please

A-And added 3 commits June 29, 2018 15:13

Full Unix test support

d37a165

Remove Test helper build from build-test

66843ce

Add msg prefix Change if condition

Change Linux URL

af8c5de

A-And requested review from jashook and BruceForstall July 2, 2018 20:39

jkotas reviewed Jul 2, 2018

View reviewed changes

Syntax error

ad40475

A-And force-pushed the FXCIUnix branch from dd81f10 to ad40475 Compare July 2, 2018 21:17

BruceForstall suggested changes Jul 3, 2018

View reviewed changes

A-And force-pushed the FXCIUnix branch 2 times, most recently from c56ce54 to ad40475 Compare July 3, 2018 21:08

A-And added 2 commits July 3, 2018 14:08

Fix Merge Conflict

0980d33

Address PR Feedback

f09ea42

A-And force-pushed the FXCIUnix branch from 4045b49 to f09ea42 Compare July 3, 2018 21:15

Remove duplicate generate_testhost call

0792329

A-And force-pushed the FXCIUnix branch from 8124769 to 0792329 Compare July 3, 2018 21:22

Merge branch 'master' into FXCIUnix

a3f4b53

BruceForstall approved these changes Jul 3, 2018

View reviewed changes

jashook reviewed Jul 3, 2018

View reviewed changes

Address PR comments

da2f8bc

A-And changed the title ~~[WIP] CoreFX CI Unix~~ CoreFX CI Unix Jul 4, 2018

Merge branch 'master' into FXCIUnix

2826e91

Clean up exclusion list

513a275

A-And force-pushed the FXCIUnix branch from 179d533 to 513a275 Compare July 10, 2018 17:58

Disable Client_ReadWriteCancelledToken_Throws_OperationCanceledException

0a2a943

A-And added 3 commits July 10, 2018 17:07

Disable all flavors of CancelledToken_Throws_OperationCanceledException

6168e97

Disable X509StoreTests.Constructor_DefaultStoreName - flaky on OSX

85f4c36

Update documentation with Linux instructions and workflow

c46d460

A-And added 2 commits July 11, 2018 16:27

Disable EventWaitHandleTests.Ctor_InvalidMode - failing on OSX

f754602

Disable Server_ReadWriteCancelledToken_Throws_OperationCanceledException

54ff5f4

A-And force-pushed the FXCIUnix branch from 8391b0a to 54ff5f4 Compare July 11, 2018 23:27

A-And merged commit e3ccef1 into dotnet:master Jul 12, 2018

A-And deleted the FXCIUnix branch July 12, 2018 04:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CoreFX CI Unix #18753

CoreFX CI Unix #18753

A-And commented Jul 2, 2018

jkotas Jul 2, 2018

A-And Jul 2, 2018

jashook commented Jul 2, 2018

A-And commented Jul 2, 2018 •

edited

Loading

BruceForstall Jul 3, 2018

A-And Jul 3, 2018

BruceForstall Jul 3, 2018

BruceForstall Jul 3, 2018

BruceForstall Jul 3, 2018

A-And Jul 3, 2018

BruceForstall commented Jul 3, 2018

jashook left a comment

jashook Jul 3, 2018

A-And Jul 3, 2018

jashook Jul 3, 2018

A-And Jul 3, 2018

jashook Jul 3, 2018

jashook Jul 3, 2018

jashook Jul 3, 2018

A-And commented Jul 9, 2018

A-And commented Jul 10, 2018

A-And commented Jul 11, 2018

A-And commented Jul 12, 2018

A-And commented Jul 12, 2018

CoreFX CI Unix #18753

CoreFX CI Unix #18753

Conversation

A-And commented Jul 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jashook commented Jul 2, 2018

A-And commented Jul 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BruceForstall commented Jul 3, 2018

jashook left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

A-And commented Jul 9, 2018

A-And commented Jul 10, 2018

A-And commented Jul 11, 2018

A-And commented Jul 12, 2018

A-And commented Jul 12, 2018

A-And commented Jul 2, 2018 •

edited

Loading