[cDAC] Draft CodeVersionsTest refactor #2
Conversation
| private IReadOnlyCollection<MockCodeBlockStart> _codeBlocks; | ||
| private Mock<IRuntimeTypeSystem> mockRuntimeTypeSystem = new(MockBehavior.Strict); | ||
| private Mock<IExecutionManager> mockExecutionManager = new(MockBehavior.Strict); | ||
| private Mock<ILoader> mockLoader = new(MockBehavior.Strict); |
There was a problem hiding this comment.
MockBehavior.Strict isn't required, but I wanted to make sure I was capturing all the calls.
| ulong addressLowBits = (ulong)address & ((ulong)_target.PointerSize - 1); | ||
|
|
||
| // this is not quite accurate on 32 bit architectures, but it's good enough for testing | ||
| ulong addressLowBits = (ulong)address & 7; |
There was a problem hiding this comment.
Should be (ulong)address & ((ulong)_target.PointerSize - 1);, however when we set this up the target isn't created. I guess we could create the target first and pass it in.
There was a problem hiding this comment.
You should be able to get the pointer size off of the TargetTestHelpers for MockDescriptor.CodeVersions, which we create before doing this.
| @@ -276,6 +157,7 @@ internal static Target CreateTarget( | |||
| [ClassData(typeof(MockTarget.StdArch))] | |||
| public void GetNativeCodeVersion_Null(MockTarget.Architecture arch) | |||
| { | |||
| mockExecutionManager.Setup(e => e.GetCodeBlockHandle(TargetCodePointer.Null)).Returns(() => null); | |||
There was a problem hiding this comment.
Only required with MockBehavior.Strict. Otherwise this test relies on the mock returning null when there is no setup. I find this a bit confusing.
|
|
||
| TargetCodePointer IExecutionManager.GetStartAddress(CodeBlockHandle codeInfoHandle) => FindCodeBlock(codeInfoHandle.Address)?.StartAddress ?? TargetCodePointer.Null; | ||
| TargetPointer IExecutionManager.GetMethodDesc(CodeBlockHandle codeInfoHandle) => FindCodeBlock(codeInfoHandle.Address)?.MethodDescAddress ?? TargetPointer.Null; | ||
| private void AddCodeBlock(MockCodeBlockStart block) |
There was a problem hiding this comment.
Do you think MockCodeBlockStart and friends are still helpful? Versus having these method just take in the address, length, method desc, etc. as separate parameters.
There was a problem hiding this comment.
I think they are still helpful to encapsulate state related to a mock. I think if we removed them, it would make tests with multiple MethodDescs confusing due to keeping track of multiple similar fields.
| internal class MockExecutionManager : IExecutionManager | ||
| { | ||
| private IReadOnlyCollection<MockCodeBlockStart> _codeBlocks; | ||
| private Mock<IRuntimeTypeSystem> mockRuntimeTypeSystem = new(MockBehavior.Strict); |
There was a problem hiding this comment.
I tend to find it easier to reason about tests when everything is contained in the test method (explicitly sets up what it needs / no state outside the method, don't have to know that each test method actually gets a new test class instance).
Maybe try something like having the test cases explicitly create the mock(s) they need and turn the Add* methods into extension methods for Mock<...>?
There was a problem hiding this comment.
Happy to make this change. I settled on having a MockExtensions class which adds the helpers as extensions on the correct generic implementation of Mock<...>.
max-charlamb
force-pushed
the
cdac-update-cv-tests
branch
from
c317e85 to
658ee65
Compare
max-charlamb
force-pushed
the
cdac-update-cv-tests
branch
from
658ee65 to
372952c
Compare
No description provided.