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 mono interpreter test leg to CI #35568

Merged
merged 13 commits into from
May 28, 2020

Conversation

steveisok
Copy link
Member

This change enables the interpreter on CI as well as providing an option for the local test run.

@ghost
Copy link

ghost commented Apr 28, 2020

Tagging subscribers to this area: @ViktorHofer
Notify danmosemsft if you want to be subscribed.

# - Windows_NT_x64
#- OSX_x64
#- Linux_arm64
- Linux_x64
Copy link
Member

Choose a reason for hiding this comment

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

It makes me wonder if we should add a new platform-matrix and consume more resources by adding more legs. Should we instead introduce a new multi-helix-job project that calls into sendtohelix.proj multiple times with different variables in parallel?

Just throwing some ideas here, that will also simplify the build whenever we move mono to build and run tests as part of the same build leg, so that way we wouldn't need to build the tests twice, etc.

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 think there are efficiencies to be had for sure. It would be nice if we could run multiple jobs like you describe and still maintain a 'separate leg' view in the UI. I feel it's important to have the interpreter be first class / distinct in that regard.

We definitely should discuss / throw out more ideas.

Copy link
Member

Choose a reason for hiding this comment

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

jobs like you describe and still maintain a 'separate leg' view in the UI

I don't think that would be possible, but I think I wouldn't mind having one leg for the mono tests and then having multiple test modes in that single run leg. our infra should be resilient to display the test results correctly for that.

Runtime tests do that, they send regular runs and non tiered compilation runs I believe.

Copy link
Member

@safern safern May 7, 2020

Choose a reason for hiding this comment

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

Discussed offline and @steveisok is going to open a issue tracking this and we can do it in a follow up PR.

Copy link
Member

Choose a reason for hiding this comment

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

What is the tracking issue? We should add it to the change here for tracking.

Copy link
Member Author

Choose a reason for hiding this comment

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

@@ -18,13 +18,20 @@
<_withoutCategories Condition="'$(ArchiveTests)' == 'true'">$(_withoutCategories);IgnoreForCI</_withoutCategories>
<_withoutCategories Condition="'$(TestScope)' == '' or '$(TestScope)' == 'innerloop'">$(_withoutCategories);OuterLoop</_withoutCategories>
<_withoutCategories Condition="!$(_withCategories.Contains('failing'))">$(_withoutCategories);failing</_withoutCategories>
<MonoEnvOptions Condition="'$(MonoEnvOptions)' == '' and '$(MonoEnableInterpreter)' == 'true'">--interpreter</MonoEnvOptions>
Copy link
Member

Choose a reason for hiding this comment

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

Is there a extensibility point to append to this env options? Like if I want to debug tests?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, that would be one. We may also want to provide extra params to the runtime. Enable logging, for example.

Copy link
Member

Choose a reason for hiding this comment

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

Let's introduce a variable or use this one to add the mono run settings.

@@ -7,6 +7,8 @@ parameters:
isOfficialBuild: false
liveRuntimeBuildConfig: ''
runtimeFlavor: 'coreclr'
runtimeDisplayName: 'coreclr'
interpreter: ''
Copy link
Member

Choose a reason for hiding this comment

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

As we discussed offline, maybe it makes sense to use the already known runtimeMode as a switch instead of adding a boolean flag: https://github.com/dotnet/runtime/blob/master/eng/pipelines/runtime.yml#L746

Copy link
Member

@safern safern left a comment

Choose a reason for hiding this comment

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

As discussed offline let's address: https://github.com/dotnet/runtime/pull/35568/files#r423356379 in a follow up PR.

@steveisok steveisok merged commit 93618ad into dotnet:master May 28, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 9, 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.

None yet

4 participants