Convert AppDomain test hooks to SystemTarget #2333
Conversation
| private static T CheckReturnBoundaryReference<T>(string what, T obj) where T : class | ||
| { | ||
| if (obj == null) return null; | ||
| if (obj is MarshalByRefObject || obj is ISerializable) |
There was a problem hiding this comment.
not sure about ISerializable here. Should it be obj.GetType().IsSerializable?
There was a problem hiding this comment.
You are probably right. Nevertheless this is old code that I just moved around and plan to deprecate it soon, not add more usages to it, since we want to get away from app domains for testing
| if (obj == null) return null; | ||
| if (obj is MarshalByRefObject || obj is ISerializable) | ||
| { | ||
| // Referernce to the provider can safely be passed across app-domain boundary in unit test process |
| public IDictionary<GrainId, IGrainInfo> GetDirectoryForTypeNamesContaining(string expr) | ||
| { | ||
| var x = new Dictionary<GrainId, IGrainInfo>(); | ||
| foreach (var kvp in ((LocalGrainDirectory)silo.LocalGrainDirectory).DirectoryPartition.GetItems()) |
There was a problem hiding this comment.
Did Silo.LocalGrainDirectory change type recently? why the cast? Same goes for Silo.Catalog.
There was a problem hiding this comment.
In the original code before the move, it was using the private field that didn't need casting. Similarly these are test hooks that were just moved but not migrated yet, and should be migrated soon. Didn't make too much effort to make sure abstractions are kept in place, since they are going away.
|
|
||
| simulatedMessageLoss[destination] = lossPercentage; | ||
|
|
||
| var mc = (MessageCenter)silo.LocalMessageCenter; |
There was a problem hiding this comment.
I wonder if this class should take these dependencies explicitly so the casts can be avoided.
There was a problem hiding this comment.
similarly, because the code is now moved to a non-nested class, it can no longer access private fields, so the casting would have to either be done in the constructor or here (or expose implementation details that we probably don't want to do for code that should eventually be removed).
| return TaskDone.Done; | ||
| } | ||
|
|
||
| public Task<long> GetReportMetricsCallCount() |
There was a problem hiding this comment.
isn't this a bit misleading if the count starts at -1?
There was a problem hiding this comment.
Agree, was just migrating existing tests. Not sure what was the reasoning, but given this took so long, I decided not to stop and analyze every test. If you tell it's OK to just use 0, I can change it
There was a problem hiding this comment.
Took a look, and I still can't make sense why it was ever -1, but still, it does seem that 0 is the right value also according to how the test which uses it is asserting, so fixed
| Assert.Equal(0, primaryActivation + secondaryActivation); | ||
|
|
||
|
|
||
| if (doLazyDeregistration) | ||
| if (lazyDeregistrationDelay > TimeSpan.Zero) | ||
| { | ||
| // Wait a bit | ||
| TimeSpan pause = lazyDeregistrationDelay.Multiply(2); | ||
| logger.Info("Pausing for {0} because DoLazyDeregistration=True", pause); |
This is so that we can eventually run the tests without the need to use AppDomains, as they are not available in .NET Core.
This PR is related to #2145