Add DelayedConstraint in NetStandard 1.6 build #2010
Conversation
| return _runComplete.WaitOne(timeout); | ||
| #endif | ||
| } |
There was a problem hiding this comment.
These two methods do the same thing on all platforms I believe, just Portable and Netstandard don't have the .WaitOne(int, bool) overload. I don't think the conditional is necessary here.
There was a problem hiding this comment.
I think the .WaitOne(int, bool) overload was missing from some platforms we no longer support.
There was a problem hiding this comment.
This looks great to me. Even though that makes two of us, I think we should get @rprouse 's comments before merging, since this is a fairly important one.
| @@ -179,7 +179,7 @@ public static ITestResult ExecuteWorkItem(WorkItem work) | |||
| // TODO: Replace with an event - but not while method is static | |||
| while (work.State != WorkItemState.Complete) | |||
| { | |||
| #if PORTABLE || NETSTANDARD1_6 | |||
| #if PORTABLE | |||
| System.Threading.Tasks.Task.Delay(1); | |||
| #else | |||
| Thread.Sleep(1); | |||
There was a problem hiding this comment.
In general, do we think it's better to use Thread.Sleep or Task.Delay where both are available? I'm wondering if we should change this generally in a future PR.
There was a problem hiding this comment.
I think it might be worth making a decision on the future of PCL, before we decide that. We previously talked about it targeting netstandard 1.0, but I'm not sure if we have much need to target that over netstandard 1.3 - which would allow us to use System.Threading - and then Thread.Sleep() across all platforms.
If we did that - I'd vote for reducing differences between platforms over any minor performance benefit Task.Delay() may give us. 😄
There was a problem hiding this comment.
That's fair enough for our framework code itself. For tests, it may be different. I replaced Thread.Sleep with Task.Delay in some of our tests to eliminate spurious failures. My general impression is that Sleep has more of a tendency to go extra long in some circumstances.
There was a problem hiding this comment.
That impression matches my understanding which is that Delay means "schedule using a precise timer" and Sleep means "I have nothing particularly important to do, deprioritize me."
| return _runComplete.WaitOne(timeout); | ||
| #endif | ||
| } |
There was a problem hiding this comment.
I think the .WaitOne(int, bool) overload was missing from some platforms we no longer support.
|
@ChrisMaddock Shall we go ahead and merge this? |
|
I think it can be - thanks! |
@rprouse - this is related to our discussion on #1638. I'd already done this as feasibility check, so thought it made sense to push it, and continue the discussion as to if we actually want to use it or not here - I'm not worried if we close instead. 😄
This re-enables DelayedConstraint in the netstandard build. It includes the
System.Threading.Threadpackage - so I also tidied up a few places whereThread.Sleep()had been excluded, as that's now available.This doesn't tackle timeout's, as I don't think Thread.Abort() is available in netstandard - notes on that are in #1638.