-
Notifications
You must be signed in to change notification settings - Fork 257
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
base: main
Are you sure you want to change the base?
DataSource draft #4112
Conversation
There was a problem hiding this 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.
src/TestFramework/TestFramework.Extensions.Csv/CsvDataSourceAttribute.cs
Outdated
Show resolved
Hide resolved
src/TestFramework/TestFramework.Extensions.Csv/CsvDataSourceAttribute.cs
Show resolved
Hide resolved
|
||
<PropertyGroup> | ||
<TargetFramework>netstandard2.0</TargetFramework> | ||
<!-- TODO: (before merge) Is this hack relevant for a new project? --> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
src/TestFramework/TestFramework.Extensions.Csv/CsvDataSourceAttribute.cs
Outdated
Show resolved
Hide resolved
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 |
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. |
There was a problem hiding this comment.
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? |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this using needed?
|
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? |
Fixes #4092