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

tests: ExecuteRunWeb: update expected SIGTERM exit code on mono. #15341

Merged
merged 2 commits into from
Feb 22, 2023

Conversation

tmds
Copy link
Member

@tmds tmds commented Jan 25, 2023

Adjust the expected exit code of the WebScenarioTests smoke test so it passes on Mono.

With this change, all smoke tests pass on mono.

dotnet/runtime#81093 tracks fixing Mono so the exit code is the same value as CoreCLR.

cc @crummel @MichaelSimons @omajid @uweigand

@tmds tmds requested a review from a team as a code owner January 25, 2023 07:45
@@ -230,5 +234,27 @@ private static string GetBinLogOption(string projectName, string command, string
return $"/bl:{Path.Combine(LogsDirectory, $"{fileName}.binlog")}";
}

private static bool DetermineIsMonoRuntime()
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'm open to suggestions on improving this method.

My first go was to check if the running .NET was using a Mono runtime, but that didn't work because my test host was actually CoreCLR (and .NET under test was mono).

Instead we need to peek in the .NET folder and find something to key off.
I'm currently using the presence of the mono-gc.h file.

Copy link
Member

Choose a reason for hiding this comment

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

another option would be to somehow flow the value of SourceBuildUseMonoRuntime (e.g. via a define) so we can use that to decide, but not sure that is really better.

Copy link
Member

Choose a reason for hiding this comment

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

IMO it doesn't matter to much on the approach. I hope to see the runtime issue resolved so there is no need for this method.

@tmds
Copy link
Member Author

tmds commented Jan 25, 2023

@MichaelSimons I'd like this to be part of 7.0.1xx, so the smoke test suite passes for the .NET 7 builds with mono. Should I target that branch? And will it flow back to main?

@MichaelSimons
Copy link
Member

@MichaelSimons I'd like this to be part of 7.0.1xx, so the smoke test suite passes for the .NET 7 builds with mono. Should I target that branch? And will it flow back to main?

Yes you should be able to target release/7.0.1xx. We will want to keep an eye out to make sure it gets merged into main.

MichaelSimons
MichaelSimons previously approved these changes Jan 25, 2023
@@ -230,5 +234,27 @@ private static string GetBinLogOption(string projectName, string command, string
return $"/bl:{Path.Combine(LogsDirectory, $"{fileName}.binlog")}";
}

private static bool DetermineIsMonoRuntime()
Copy link
Member

Choose a reason for hiding this comment

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

IMO it doesn't matter to much on the approach. I hope to see the runtime issue resolved so there is no need for this method.

crummel
crummel previously approved these changes Jan 25, 2023
@tmds tmds changed the base branch from main to release/7.0.1xx January 26, 2023 17:19
@tmds tmds dismissed stale reviews from crummel and MichaelSimons January 26, 2023 17:19

The base branch was changed.

@tmds tmds requested a review from rbhanda as a code owner January 26, 2023 17:19
@tmds
Copy link
Member Author

tmds commented Jan 26, 2023

I've changed the target branch to 7.0.1xx, so it doesn't get merged on main now.
I'll rebase my branch tomorrow so it becomes mergable again

@tmds
Copy link
Member Author

tmds commented Jan 27, 2023

I've rebased on 7.0.1xx, the commit hasn't changed. It should be good to merge, after the CI run.

@tmds
Copy link
Member Author

tmds commented Jan 30, 2023

The CI issue doesn't look related.

@MichaelSimons
Copy link
Member

The CI issue doesn't look related.

Agreed. This missed the Feb servicing window and will be merged for March once the branch opens.

@tmds
Copy link
Member Author

tmds commented Feb 22, 2023

Can this be merged now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants