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

DataSource draft #4112

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

Youssef1313
Copy link
Member

@Youssef1313 Youssef1313 commented Nov 21, 2024

Fixes #4092

@Youssef1313 Youssef1313 changed the title Implement CsvDataSourceAttribute DataSource draft Nov 21, 2024
Copy link
Member

@Evangelink Evangelink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the name of the package, I was thinking about something like MSTest.Data.Csv or MSTest.DataSource.Csv or because we go for a functionally oriented name maybe the NuGet package could be tuned around the technology used so MSTest.Data.Oledb or MSTest.DataSource.Oledb. WDYT?

As for the name of the dll, my plan for v4 is to simplify naming so I think (if there is no technical constraint) we should do the same for these new packages.


<PropertyGroup>
<TargetFramework>netstandard2.0</TargetFramework>
<!-- TODO: (before merge) Is this hack relevant for a new project? -->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question, it's probably fine as this is (was?) mainly for some VS binding redirects.

Copy link
Member Author

@Youssef1313 Youssef1313 Nov 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Evangelink by fine, do you mean it can be removed for the new projects? or do you mean just keep as-is? I don't have much context on what issues is it trying to solve

@Youssef1313
Copy link
Member Author

For the name of the package, I was thinking about something like MSTest.Data.Csv or MSTest.DataSource.Csv or because we go for a functionally oriented name maybe the NuGet package could be tuned around the technology used so MSTest.Data.Oledb or MSTest.DataSource.Oledb. WDYT?

To me, the fact that we use OleDb for Csv is an implementation detail that can (and likely will) change in future. And for XML, we are not using OleDb at all and don't have extra dependencies. I think MSTest.DataSource.[Csv|Xml|Database] may be better (not very certain about "Database" yet though)

using OleDbConnection connection = new();

// We have to use the name of the folder which contains the CSV file in the connection string
// If target platform is x64, then use CsvConnectionTemplate64 connection string.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will this work also on arm64?


IEnumerable<object?[]> ITestDataSource.GetData(MethodInfo methodInfo)
{
// TODO: Avoid using OleDb and instead parse Csv directly. Maybe use https://www.nuget.org/packages/CsvHelper? Write our own?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would definitely avoid writing our own. 😁

@@ -52,6 +57,7 @@ public class DataSourceTests : AcceptanceTestBase


#file MyTestClass.cs
using System.Data;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this using needed?

@nohwnd
Copy link
Member

nohwnd commented Nov 21, 2024

MSTest.DataSource.Csv sounds good. Maybe MSTest.DataSource.OleDb.Csv would be better? Or something that makes it less generic so we can replace it with other better solution in the future?

@Youssef1313
Copy link
Member Author

MSTest.DataSource.Csv sounds good. Maybe MSTest.DataSource.OleDb.Csv would be better? Or something that makes it less generic so we can replace it with other better solution in the future?

My view here is that if we wanted to replace OleDb with something else, it should be done in the same package, not as a new package. To me, the fact that we use OleDb is very implementation detail. Are you thinking of any scenarios where the user will still want to use OleDb with Csv if we provided an alternative?

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

Successfully merging this pull request may close these issues.

RFC011 - Custom data sources
3 participants