[cdac] Make contracts tests use mock/placeholder target instead of actual target#110027
Conversation
…ory/target) into separate files and subfolder
elinor-fung
added
test-enhancement
| .ToDictionary(); | ||
|
|
||
| // Tests are currently always set to use funclets | ||
| bool useFunclets = true; |
|
|
||
| namespace Microsoft.Diagnostics.DataContractReader.Tests; | ||
|
|
||
| internal partial class MockDescriptors |
There was a problem hiding this comment.
This file was just renamed from ExecutionManagerTestBuilder.cs and put as an inner class of MockDescriptors. The only actual change was #110027 (comment)
|
|
||
| namespace Microsoft.Diagnostics.DataContractReader.Tests.ContractDescriptor; | ||
|
|
||
| internal class ContractDescriptorBuilder : MockMemorySpace.Builder |
There was a problem hiding this comment.
This was all copied out of MockMemorySpace.cs
|
|
||
| namespace Microsoft.Diagnostics.DataContractReader.Tests.ContractDescriptor; | ||
|
|
||
| internal unsafe class ContractDescriptorHelpers |
There was a problem hiding this comment.
This was all copied out of TargetTestHelpers.cs
| @@ -19,21 +21,19 @@ public void GetPath(MockTarget.Architecture arch) | |||
| TargetTestHelpers helpers = new(arch); | |||
| MockMemorySpace.Builder builder = new(helpers); | |||
| MockLoader loader = new(builder); | |||
There was a problem hiding this comment.
By switching from creating a "real" ContractDescriptorTarget to a TestPlaceholderTarget, we are bypassing the ContractDescriptorParser. It looks like we already test the contractdescriptor reading in other places, is the benefit that it is easier to mock out contracts?
There was a problem hiding this comment.
Yeah, the intent is that it is easier to mock out contracts (or other things needed by tests). It narrows down the portion of the 'real' product we are testing and we don't need to build out the JSON needed for the contract descriptor (which we were never using for a bunch of the tests already using the placeholder target).
|
|
||
| internal Dictionary<DataType, Target.TypeInfo> Types { get; } | ||
| internal (string Name, ulong Value, string? Type)[] Globals { get; } | ||
| internal (string Name, ulong Value)[] Globals { get; } |
There was a problem hiding this comment.
Why is the string? Type no longer needed for globals when testing? Looking at how they are used in the contracts, it seems the contracts are assumed to already know the type and reference them by name.
It looks like the ContractDescriptor JSON encodes the global type information and it is returned when reading a global. I don't see any contracts that use this, but it is part of the available information, is this needed? If so, do we need to replicate it during testing?
There was a problem hiding this comment.
We put the global type information (optionally) in the contract descriptor with the idea that contracts could validate what type they expect. But as you noted, we didn't actually end up doing this in the contracts we added.
We have testing for ContractDescriptorTarget that covers getting the global type information (when it exists). For all the mock descriptors that are now used with the placeholder target, I removed the optional type to make the test infrastructure a little simpler.
…tual target (dotnet#110027) - Separate helpers for testing an actual contract descriptor from helpers for mock memory - Tests/helpers for the contract descriptor are now under the `ContractDescriptor` folder - Make all contracts tests use the mock target instead of creating a contract descriptor and using the `ContractDescriptorTarget` - Update root namespaces from `Microsoft.Diagnostics.DataContractReader.UnitTests` -> `Microsoft.Diagnostics.DataContractReader.Tests` to match assembly name
3 participants
ContractDescriptorfolderContractDescriptorTargetMicrosoft.Diagnostics.DataContractReader.UnitTests->Microsoft.Diagnostics.DataContractReader.Teststo match assembly name