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

RFC011 - Custom data sources #4092

Open
1 of 4 tasks
Youssef1313 opened this issue Nov 20, 2024 · 8 comments · May be fixed by #4112
Open
1 of 4 tasks

RFC011 - Custom data sources #4092

Youssef1313 opened this issue Nov 20, 2024 · 8 comments · May be fixed by #4112

Comments

@Youssef1313
Copy link
Member

Youssef1313 commented Nov 20, 2024

RFC 011 - Custom data sources

  • Approved in principle
  • Under discussion
  • Implementation
  • Shipped

Summary

A replacement for DataSourceAttribute, which is currently .NET Framework only, to make it more modern and bring the support for .NET Core.

Motivation

User needs for DataSource to be supported on .NET Core. #233

Detailed design

Instead of having a single giant attribute like DataSourceAttribute, we will divide it into multiple attributes.

XmlDataSourceAttribute

The attribute constructor will have three parameters, (string xmlFilePath, string tableName)

Previously with DataSrouceAttribute, the code used to look like this:

[TestMethod]
[DeploymentItem("MyTests.Configuration.xml")]
[DataSource("Microsoft.VisualStudio.TestTools.DataSource.XML", "|DataDirectory|\\MyTests.Configuration.xml", "MyTable1", DataAccessMethod.Sequential)]
public void TestMethod()
{
    var foo = TestContext.DataRow["MyData"]);
}

XML file:

<?xml version="1.0" encoding="utf-8" ?>
<Rows>
  <MyTable1>
    <MyData>true</MyData>
  </MyTable1>
  <MyTable1>
    <MyData>false</MyData>
  </MyTable1>

  <MyTable2>
    <MyData2>false</MyData2>
  </MyTable2>
</Rows>

Now, it will be:

[TestMethod]
[XmlDataSource("MyTests.Configuration.xml", "MyTable1")]
public void TestMethod(bool myData /*parameters can't be validated at compile-time*/)
{
    // The parameters are the alternative of TestContext.DataRow["MyData"]);
}

The attribute will implement ITestDataSource and may be in a separate package.

CSVDataSourceAttribute

The attribute constructor will have one parameters, (string csvFilePath)

Previously with DataSrouceAttribute, the code used to look like this:

[TestMethod]
[DataSource("Microsoft.VisualStudio.TestTools.DataSource.CSV", "TestData.csv", "TestData#csv", DataAccessMethod.Sequential)]
public void TestMethod()
{
    var header1 = TestContext.DataRow["CsvHeader1"]);
}

Now, it will be:

[TestMethod]
[CSVDataSource("TestData.csv")]
public void TestMethod(string header1, int header2, ... /*parameters can't be validated at compile-time*/)
{
    // The parameters are the alternative of TestContext.DataRow["CsvHeader1"]);
}

The attribute will implement ITestDataSource and may be in a separate package.

SqlDataSourceAttribute

TBD

Concerns:

  • Security: What's the best way to store connection strings?

Drawbacks

Why should we not do this?

Alternatives

What other designs have been considered? What is the impact of not doing this?

Compatibility

This change isn't a breaking change. But it brings functionality to .NET ("Core") that is done differently on .NET Framework

Unresolved questions

What parts of the design are still TBD?

@Evangelink
Copy link
Member

Linking https://github.com/microsoft/testfx/blob/main/docs/RFCs/007-DataSource-Attribute-VS-ITestDataSource.md.

Ideally we want the new attribute to implement the same interface as the other source attributes. Outside of technical constraints, we would also like to unfold arguments and declare them on the method signature instead of using the TestContext.DataRow[".."] syntax.

Finally, I think we should drop the DataAccessMethod and consider these sources the same way we consider other parameterized tests.

@Youssef1313
Copy link
Member Author

@Evangelink Thanks. I updated the proposal a little bit.

@Youssef1313
Copy link
Member Author

One consideration for not having TestContext.DataRow[...] is how the users are using data sources. Are there any valid scenarios where the XML could contain much more data than needed?

If we go with parameters instead of TestContext.DataRow, users will have to declare everything in the correct order, and it will be noisy if part of the schema is unused for the purpose of the test. I'm not sure if that's how users are using DataSource though.

In that case, we may be able to have the test parameter as DataRow? e.g:

[TestMethod]
[XmlDataSource("MyTests.Configuration.xml", "MyTable1")]
public void TestMethod(DataRow dataRow /*alternative of TestContext.DataRow*/)
{
}

@Evangelink
Copy link
Member

That's a good remark, we probably need wrapper type yeah.

@Youssef1313
Copy link
Member Author

@Evangelink Is it "our own new wrapper type"? Or could we just use "DataRow"? (available in all .NET versions without extra packages)

@Youssef1313 Youssef1313 linked a pull request Nov 21, 2024 that will close this issue
@fforjan
Copy link

fforjan commented Nov 21, 2024

My 5 cents, from our side and our migration to .net 8 (and continue building .net framework in parallel of...)

We took the following approach:
Image

The XML file looks like :
Image

The intent was, sometimes the data in the XML need to be parsed differently :
Image

The base class look like this
Image
and the GetData on the datasource folder:
Image

Which then the test look likes:
Image

@Evangelink
Copy link
Member

Thanks for your input @fforjan. I read it as our solution wouldn't be helping you much, am I right?

Giving some more thoughts on it, I am actually wondering if this feature is actually bringing much to our users.

I mean that either we will end up in a too generic solution where user is basically left with a too generic argument type or something too narrow where the user ends up having to write its own extension.

For XML, I don't think we are really providing much value as (nearly) everything is already available out-of-the-box. For CSV, we would need to either re-implement a parser which is a no-go or rely on some library from community (with licensing/security/support concerns) but even there, I have the feeling we will either have too generic type or too narrow which means we don't have the right abstraction. Finally, for databases, this seems to be even more true.

I am more tempted to invest some time in offering a different fixture experience that would give the flexibility to use the library and srong typing the user wants.

I'll sleep on it.

@fforjan
Copy link

fforjan commented Nov 21, 2024

I read it as our solution wouldn't be helping you much, am I right?
yes, the painful part in my solution was to implement the GetData method and as we were successful, we didn't even think you were looking to port (and raise such request!) the original DataSourceAttribute from the .net Framework - we were using it but had to move away to allow the tests to run in both fwk and core.

In our case, we decided that the xml file would be read from the filesystem as such, relative to the cs:
Image
We could have decided to read the file where the DLLs is executed or we could also have read the xml file as a stream from a resource from an assembly. (in the end, would the DataSourceAttribute consider the reference as a DeploymentItem implicit reference too, would the file be an explicit deployment item too?)

_ I have the feeling we will either have too generic type or too narrow which means we don't have the right abstraction. Finally, for databases, this seems to be even more true._
I would agree with you, same if you unit test is a sqllite file, where do you read it (see the previous remark on the deploy item) ? Where do you store the credential for a real DB ? etc.. I think everyone would have a different answer.

What would be helpful potentially is more end to end sample/simple for the different use case - XML/CSV/DB- which people can adapt from ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants