Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow IEnumerable of tuple for DynamicData source #2226

Merged
merged 1 commit into from
Feb 1, 2024

Conversation

Evangelink
Copy link
Member

To avoid breaking public API, we decided to convert the tuple in object[] to preserve it. The current solution relies on ITuple which is only available in NET471+. Given we currently target net462 for full framework, that means that the feature will only be available for netcore. I'll weigh the benefit/cost ratio of adding net471 build
in the package so this feature can be available more widely.

Fixes #1335 and fixes #1624

To avoid breaking public API, we decided to convert the tuple in
`object[]` to preserve it. The current solution relies on `ITuple`
which is only available in NET471+. Given we currently target net462
for full framework, that means that the feature will only be available
for netcore. I'll weigh the benefit/cost ratio of adding net471 build
 in the package so this feature can be available more widely.
@nohwnd
Copy link
Member

nohwnd commented Feb 1, 2024

I think it makes sense.

  1. how do you guard that data that currently contain tuple are not expanded in unwanted way? Maybe there should be a flag that keeps it working as is?

  2. Do you think it would make sense to add a similar "container" that is generic as the Typed Theory Data. Because ensuring that the output of the data source fits into the parameters of the test method needs to be done via an analyzer. This allows users to define their own types that have simpler constructors, but also might trap us into having to analyze all output types with inheritance. So probably no, and we just stick with tuples?

  3. also please plan for analyzer that connects those pieces together. Because that is imho half of the value. I know the linked issue says that with tuples the error will be caught on compile time, but I don't see how that would be possible without an analyzer. I don't think there is any way to directly link attribute to be "spread" over method signature. But I might be wrong :))

@Evangelink
Copy link
Member Author

  1. how do you guard that data that currently contain tuple are not expanded in unwanted way? Maybe there should be a flag that keeps it working as is?

Not entirely sure to get the issue you are describing. Do you have some example?

  1. Do you think it would make sense to add a similar "container" that is generic as the Typed Theory Data. Because ensuring that the output of the data source fits into the parameters of the test method needs to be done via an analyzer. This allows users to define their own types that have simpler constructors, but also might trap us into having to analyze all output types with inheritance. So probably no, and we just stick with tuples?

Yes, I think it makes sense as alternative, see #2227. We did something similar in TATF, it's also making it easier to provide custom name for each entry if we want.

  1. also please plan for analyzer that connects those pieces together. Because that is imho half of the value. I know the linked issue says that with tuples the error will be caught on compile time, but I don't see how that would be possible without an analyzer. I don't think there is any way to directly link attribute to be "spread" over method signature. But I might be wrong :))

#2237

@nohwnd
Copy link
Member

nohwnd commented Feb 1, 2024

  1. And existing method that is emulating what we do and outputs single item array with a tuple in it, and then deconstructs that in the test method. This change will break my code because my tuple will try to bind to 2 parameters instead of 1. Or no?
namespace TestProject74
{
    [TestClass]
    public class UnitTest1
    {
        [DynamicData(nameof(GetData))]
        [TestMethod]
        public void TestMethod1((string file, string mode) t)
        {
            Console.WriteLine(t.file);
        }

        public IEnumerable<object[]> GetData()
        {
            return new[]
            {
                new object[] { (file: "aaa.txt", mode: "write") }
            };
        }
    }
}

@Evangelink
Copy link
Member Author

I expect that this would still work from what I have seen but I will add a test to ensure that's the case. Thanks!

@Evangelink
Copy link
Member Author

I have done some manual testing and we are good. I won't add integration test here because we will need to rewrite our integration suite to allow this test and I would prefer to postpone this change for a different PR.

@Evangelink Evangelink merged commit dc8f6aa into microsoft:main Feb 1, 2024
7 checks passed
@Evangelink Evangelink deleted the dynamic-data-tuple branch February 1, 2024 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants