[cdac][tests] Make it easier to set up a unit test #108288
Conversation
Now that Target's reader callback is just a normal managed delegate, we don't have a lot of reason to make the testing ReadContext a ref struct. So simplify our resource usage and just store the descriptors and json as heap allocated arrays
The ReadContext for the tests keeps a copy of the json payload, we don't need to use unmanaged pointers
we don't hold pointers to span data anymore
most of our tests that use a valid contract descriptor don't need to repeat the boilerplat to fill it in
We previously did the wrong thing with TargetTestHelpers.MakeGlobalJson: it always filled in direct values. So technically filling in the pointer data in all the other tests is unnecessary. Add a second SetGlobals overload for the builder that takes a 4-tuple (either ulong? Value or uint IndirectIndex) and a second collection of actual values for the indices.
everything is using high level construction now
dotnet-issue-labeler
bot
added
the
needs-area-label
lambdageek
added
area-Diagnostics-coreclr
and removed
needs-area-label
|
Tagging subscribers to this area: @tommcdon |
| if (_heapFragments.Count > 0) | ||
| Span<byte> descriptor = stackalloc byte[_targetTestHelpers.ContractDescriptorSize]; | ||
| _targetTestHelpers.ContractDescriptorFill(descriptor, jsonLength, pointerDataCount); | ||
| const ulong ContractDescriptorAddr = 0xaaaaaaaa; |
There was a problem hiding this comment.
Is this an important value? The address is also only 32-bits and we are assigning to a 64-bit value. Perhaps a comment to clarify what this is used for?
There was a problem hiding this comment.
Moved it to the top of the file and added a comment that it's arbitrary but mustn't overlap any of the other arbitrary addresses that the tests use.
Right now in the interest of not doing too much magic and keeping the tests reproducible all the "allocation" in the mock memory space is done by hand - by picking an address that doesn't overlap anything and allocating a fragment there (the builder will tell you if you mathed wrong). I think in the longer term it will be easier to write bigger tests if we had some kind of mock malloc, but I'm worried that debugging a failed test will be too difficult, or the tests might become non-deterministic or something. But on the other hand, I'm hoping after the contract refactoring we won't have to write massive tests. So we'll see.
| throw new InvalidOperationException("Context already created"); | ||
| GCHandle fragmentReaderHandle = default; ; | ||
| if (_heapFragments.Count > 0) | ||
| Span<byte> descriptor = stackalloc byte[_targetTestHelpers.ContractDescriptorSize]; |
There was a problem hiding this comment.
This seems odd. Why not just allocate the array and then assign it below. Both the stackalloc and the array seem unnecessary.
There was a problem hiding this comment.
yea I missed these while copy/pasting. They're leftover from the earlier unsafe pointer based approach.
I checked, there aren't any more stackalloc in the tests
| if (_indirectValues != null) | ||
| { | ||
| int pointerSize = _targetTestHelpers.PointerSize; | ||
| Span<byte> pointerDataBytes = stackalloc byte[_indirectValues.Count * pointerSize]; |
There was a problem hiding this comment.
Another stackalloc and array question.
We used to stackalloc a lot of data because the Target callback had to be an UnmanagedCallersOnly function pointer. But now it's just a delegate. So we can heap allocate the mock memory space and get rid of a bunch of incidental fixed statements We used to spell out the JSON contract descriptor in every test case. We dont' need to do that - most tests are not testing parsing. Hide all that stuff in MockMemorySpace.Builder * make MockMemorySpace.ReadContext a class Now that Target's reader callback is just a normal managed delegate, we don't have a lot of reason to make the testing ReadContext a ref struct. So simplify our resource usage and just store the descriptors and json as heap allocated arrays * remove uses of fixed in tests The ReadContext for the tests keeps a copy of the json payload, we don't need to use unmanaged pointers * make the MockMemorySpace.Builder a normal class we don't hold pointers to span data anymore * make MockMemorySpace.ReadContext private * begin hiding ContractDescriptorFill most of our tests that use a valid contract descriptor don't need to repeat the boilerplate to fill it in * start removing json and pointer data boilerplate * Don't fill the pointer data when there isn't any We previously did the wrong thing with TargetTestHelpers.MakeGlobalJson: it always filled in direct values. So technically filling in the pointer data in all the other tests is unnecessary. Add a second SetGlobals overload for the builder that takes a 4-tuple (either ulong? Value or uint IndirectIndex) and a second collection of actual values for the indices. * remove unused MockMemorySpace.Builder functions everything is using high level construction now * remove unneeded stackalloc * describe test addresses
Targetcallback had to be an UnmanagedCallersOnly function pointer. But now it's just a delegate. So we can heap allocate the mock memory space and get rid of a bunch of incidentalfixedstatementsMockMemorySpace.Builder