Offer alternative to serialization of data #4467
Conversation
| /// Defines the strategy for uniquely identifying test data. | ||
| /// This is only used when <see cref="ITestDataSourceUnfoldingCapability.UnfoldingStrategy" /> is set to <see cref="TestDataSourceUnfoldingStrategy.Unfold"/>. | ||
| /// </summary> | ||
| public interface ITestDataIdentifierStrategy |
There was a problem hiding this comment.
Given this enum and interface only work when using the unfolding strategy, I was wondering if we should merge the 2 together so we have something like Auto, Fold, UnfoldUsingDataContractSerialization, UnfoldUsingDataIndex.
WDYT?
There was a problem hiding this comment.
@Evangelink You mean not needing the interface at all, and only relying on the existing enum? This is fine for me.
We will have to keep Unfold and make it the same as UnfoldUsingDataContractSerialization for backcompat I assume. But that's fine.
There was a problem hiding this comment.
Yes that's what I meant. My last commit is doing the change, please review and let me know if you think that's better or worse.
| { | ||
| discoveredTest.TestMethod.SerializedData = DataSerializationHelper.Serialize(d); | ||
| discoveredTest.TestMethod.SerializedData = new string[3] |
There was a problem hiding this comment.
I am reusing the serialized data property as it's not used and allows me to avoid adding more properties to the TestCase object as it has some perf impact. This isn't perfect but as far as I can tell, there is no way for the serialized data to be serialized this way as we prefix with the data type.
There was a problem hiding this comment.
Fine for me to reuse. At least until we have a strong reason not to reuse.
| throw new InvalidOperationException(); | ||
| } | ||
|
|
||
| dataSource = _testMethodInfo.GetAttributes<Attribute>(false)?.OfType<UTF.ITestDataSource>().Skip(dataSourceIndex).FirstOrDefault(); |
There was a problem hiding this comment.
It may be a good idea to check with runtime team that it's safe to assume reflection will return attributes in a deterministic order. I think it should be a safe assumption, but just a sanity check.
There was a problem hiding this comment.
From my tests it is deterministic but yes I'll double check with them.
| /// <summary> | ||
| /// Identifies test data by its index in the data source. | ||
| /// </summary> | ||
| DataIndex, |
There was a problem hiding this comment.
In the context of HotReload, if an attribute is removed, re-ordered, etc, is everything going to work fine?
There was a problem hiding this comment.
Will double check that scenario!
src/TestFramework/TestFramework/PublicAPI/PublicAPI.Unshipped.txt
Outdated
Show resolved
Hide resolved
src/TestFramework/TestFramework/PublicAPI/PublicAPI.Unshipped.txt
Outdated
Show resolved
Hide resolved
src/TestFramework/TestFramework/PublicAPI/PublicAPI.Unshipped.txt
Outdated
Show resolved
Hide resolved
| || !int.TryParse(_test.SerializedData[1], out int dataSourceIndex) | ||
| || !int.TryParse(_test.SerializedData[2], out int dataIndex)) |
There was a problem hiding this comment.
Consider using the IFormatProvider overload and pass CultureInfo.InvariantCulture.
There was a problem hiding this comment.
I'm surprised there was no CA warning here. I need to verify the existing rules.
| /// Each data row is treated as a separate test case, and the data is unfolded using | ||
| /// <see cref="System.Runtime.Serialization.Json.DataContractJsonSerializer"/>. | ||
| /// </summary> | ||
| UnfoldUsingDataContractJsonSerializer, |
There was a problem hiding this comment.
Should we make this the same numeric value as Unfold?
| /// </summary> | ||
| Auto, | ||
|
|
||
| /// <summary> | ||
| /// Each data row is treated as a separate test case. | ||
| /// </summary> | ||
| [Obsolete("Use 'UnfoldUsingDataContractJsonSerializer' instead")] |
There was a problem hiding this comment.
Consider EditorBrowsable(Never), and maybe also inheritdoc from UnfoldUsingDataContractJsonSerializer
This pull request includes significant changes to the
MSTest.TestAdapterandTestFrameworkto introduce a new strategy for identifying test data in data-driven tests. The changes involve updating the test discovery and execution logic to support the newTestDataIdentifierStrategy.Fixes #1462
IMPORTANT: when using the data index strategy any impact on the order of the data will impact the index which in turn will impact the test ID. This can have important side effect if you are using a system tracking the test ID over time.
Key changes include:
Enhancements to Test Discovery:
EnumerateAssemblymethod inAssemblyEnumerator.csto include the newTestDataIdentifierStrategywhen discovering tests. [1] [2] [3]DiscoverTestsInTypemethod to pass theTestDataIdentifierStrategyto the test discovery process. [1] [2]Enhancements to Test Execution:
TryExecuteITestDataSourceinTestMethodRunner.csto handle the execution of tests using the new identifier strategy. [1] [2]Interface and Attribute Updates:
ITestDataIdentifierStrategyinterface and theTestDataIdentifierStrategyenum to define the strategy for uniquely identifying test data.DataRowAttributeandDynamicDataAttributeto implement the newITestDataIdentifierStrategyinterface. [1] [2] [3] [4]TestDataSourceOptionsAttributeto include the new identifier strategy.TODOs: