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.

Sign up for GitHub

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

Jump to bottom

Make RemoteExecutor throw RemoteExecutorNotSupportedException for size restricted workloads #5401

Merged
merged 3 commits into from
May 7, 2020
Merged

Make RemoteExecutor throw RemoteExecutorNotSupportedException for size restricted workloads #5401

merged 3 commits into from
May 7, 2020

Conversation

steveisok
Copy link
Member

@steveisok steveisok commented May 4, 2020 •
edited
Loading

Because size restricted workloads typically do not allow spawning processes, we will throw SkipTestException for iOS, tvOS, watchOS, and android. The xharness test runner will also need to be modified in order to fully skip tests that are impacted by this change. Another PR will be filed to take care of that.

Steve Pfister added 2 commits May 4, 2020 10:52
Make RemoteExecutor throw RemoteExecutorNotSupportedException for siz…
eead3f5 
…e restricted workloads

Because size restricted workloads typically do not allow spawning processes, we will throw
RemoteExecutorNotSupportedExceptions for iOS, tvOS, watchOS, and android.  We will also note the tests
as skipped through SkippedTestMessageBus.
Additional os checks
6127d34
@steveisok
Copy link
Member Author

#5129

stephentoub
stephentoub reviewed May 4, 2020
View reviewed changes
src/Microsoft.DotNet.XUnitExtensions/src/SkippedTestMessageBus.cs Outdated
@@ -36,7 +36,7 @@ public bool QueueMessage(IMessageSinkMessage message)
if (testFailed != null)
{
var exceptionType = testFailed.ExceptionTypes.FirstOrDefault();
if (exceptionType == typeof(SkipTestException).FullName)
if (exceptionType == typeof(SkipTestException).FullName || exceptionType == "Microsoft.DotNet.RemoteExecutor.RemoteExecutorNotSupportedException")
Copy link
Member

Choose a reason for hiding this comment

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

I was under the impression the SkipTestException logic only applied to tests marked as ConditionalFact/Theory. Is that not the case? If it is the case, is the plan to change all of those tests to be ConditionalFact/Theory and to require that of any test that uses RemoteExecutor?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's my understanding as well. I'm under the impression that we will need to go in and add ConditionalFact/Theory to the right places.

safern
safern reviewed May 4, 2020
View reviewed changes
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.

So this means that now all tests that use RemoteExecutor need to use ConditionalFact/Theory?

Should we also add the handling of the skipped test when a SkipTestException is thrown to the xharness test runners?

@safern
Copy link
Member

safern commented May 4, 2020

Should we also add the handling of the skipped test when a SkipTestException is thrown to the xharness test runners?

Maybe, here: https://github.com/dotnet/xharness/blob/master/src/Microsoft.DotNet.XHarness.Tests.Runners/xUnit/XUnitTestRunner.cs#L222?

@steveisok
Copy link
Member Author

So this means that now all tests that use RemoteExecutor need to use ConditionalFact/Theory?

Should we also add the handling of the skipped test when a SkipTestException is thrown to the xharness test runners?

I assume adding SkipTestException to the test runners would not require ConditionalTheory/Fact opt in? If so, it might make more sense because skipping RemoteExecutor tests applies to the whole platform. Opting in one by one doesn't seem to have as much value.

@safern
Copy link
Member

safern commented May 4, 2020

Yeah, I think we should add support for this to the xharness xunit runner. For example I think we can add something similar to: https://github.com/dotnet/arcade/blob/master/src/Microsoft.DotNet.XUnitExtensions/src/SkippedTestMessageBus.cs#L41

Into https://github.com/dotnet/xharness/blob/master/src/Microsoft.DotNet.XHarness.Tests.Runners/xUnit/XUnitTestRunner.cs#L114

Where we check if we're handling a test failure, look at the exception type, if it is SkipTestException then instead of calling actualHandler, we call HandleTestSkipped: https://github.com/dotnet/xharness/blob/master/src/Microsoft.DotNet.XHarness.Tests.Runners/xUnit/XUnitTestRunner.cs#L134

@steveisok
Copy link
Member Author

Make sense

@steveisok steveisok merged commit e777db7 into dotnet:master May 7, 2020
@stephentoub stephentoub mentioned this pull request May 27, 2020
Closed
5 tasks
@akoeplinger akoeplinger mentioned this pull request Jun 4, 2020
Merged
akoeplinger added a commit that referenced this pull request Jun 4, 2020
@akoeplinger
Add RemoteExecutor.IsSupported property instead of SkipTestException (#…
c4e5396 
…5609)

Integrating the SkipTestException added in #5401 proved too difficult.
Instead we'll go with annotating tests with ConditionalFact/Theory and using this property to skip.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ViktorHofer ViktorHofer ViktorHofer left review comments

@stephentoub stephentoub stephentoub left review comments

@safern safern safern approved these changes

@akoeplinger akoeplinger Awaiting requested review from akoeplinger

@danmoseley danmoseley Awaiting requested review from danmoseley

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@steveisok @safern @stephentoub @ViktorHofer