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

DataRow: Nulls and empty string are displayed as no data in display name #1647

Closed
Evangelink opened this issue Apr 28, 2023 · 9 comments · Fixed by #3082
Closed

DataRow: Nulls and empty string are displayed as no data in display name #1647

Evangelink opened this issue Apr 28, 2023 · 9 comments · Fixed by #3082

Comments

@Evangelink
Copy link
Member

Evangelink commented Apr 28, 2023

Describe the bug

The display name of MSTest DataRow elements doesn't display well nulls.

Steps To Reproduce

[DataRow("", "", false)]
[DataRow(null, null, false)]
public void M1(string s1, string s2, bool b)
{
}

Expected behavior

Should be displayed as

M1 ("","",false)
M1 (null,null,false)

Actual behavior

Will be displayed as

M1 (,,false)
M1 (,,false)

Additional context

The empty string should also probably be improved to display "" instead of empty.

@Evangelink Evangelink added Help-Wanted The issue is up-for-grabs, and can be claimed by commenting Type: Feature Area: Parameterized tests State: Approved and removed Needs: Triage 🔍 labels Apr 28, 2023
@Evangelink Evangelink added this to the 3.1.0 milestone Jun 5, 2023
@Evangelink Evangelink changed the title DataRow: Nulls are displayed as no data in display name DataRow: Nulls and empty string are displayed as no data in display name Jul 10, 2023
@Evangelink
Copy link
Member Author

Moved to milestone 3.2

@Evangelink Evangelink modified the milestones: 3.1.0, 3.2.0 Jul 10, 2023
@Evangelink Evangelink removed this from the 3.2.0 milestone Dec 7, 2023
@Evangelink Evangelink added this to the 4.0.0 milestone Jan 2, 2024
@Evangelink
Copy link
Member Author

This can be a breaking change for people using the ID generation with display mode (although it is deprecated it's still in place so we will do th breaking change in v4).

@nohwnd
Copy link
Member

nohwnd commented Jan 23, 2024

If we are breaking this, I would like to something along the type serialization providers.

namespace mstest2;

[TestClass]
public class UnitTest1
{
    [TestMethod]
    [DataRow("a", 'b', "null", null, "", new string[0], new string []{"a","b" })]
    public void TestMethod1(object a, object b, object c, object d, object e)
    {
    }
}

Will output TestMethod1 (a,b,null,,,System.String[],System.String[]).

I would much rather see something like TestMethod1 ("a", 'b', "null", null, "", System.String[], System.String["a", "b"]). This would also help IDataSource, where you have to serialize the data "from scratch" in every new provider, and you cannot easily leave the parameter serialization up to the framework and only provide your own new name for the test.

@Kissaki
Copy link

Kissaki commented Jun 5, 2024

The change was mentioned to be a breaking change, so I assume this ticket should be linked to from #1285 MSTest v4 Breaking Changes?

@Evangelink
Copy link
Member Author

With the change done in #3053 we can avoid the breaking change and easily fix this ticket. @Kissaki would you like to give it a go? Or maybe @MichelZ?

@MichelZ
Copy link
Contributor

MichelZ commented Jun 10, 2024

With the change done in #3053 we can avoid the breaking change and easily fix this ticket. @Kissaki would you like to give it a go? Or maybe @MichelZ?

Something like this? #3082 (689fa0b) Altough this does not take into account the comments from @nohwnd

@Evangelink
Copy link
Member Author

I have merged your other PR so you can rebase! If you want to also handle string (by surrounding with ") and char (by surround with ') that's awesome. Otherwise we can do another ticket/PR.

@Evangelink Evangelink modified the milestones: 4.0.0, 3.5.0 Jun 10, 2024
@MichelZ
Copy link
Contributor

MichelZ commented Jun 10, 2024

I have merged your other PR so you can rebase! If you want to also handle string (by surrounding with ") and char (by surround with ') that's awesome. Otherwise we can do another ticket/PR.

Does an "empty char" even exist? I did a rebase on the branch, and string is handled, yes

@Evangelink
Copy link
Member Author

Does an "empty char" even exist? I did a rebase on the branch, and string is handled, yes

I didn't mean empty char. I meant that from the first read of your PR, you have been handling:

  • null displayed as null
  • empty string displayed as ""

I was suggesting to also improve:

  • "aaa" to be displayed as "aaa" and not as aaa as it is today
  • and 'a' as 'a' and not as a.

Basically, all strings should be surrounded by double quotes and all chars should be surrounded by single quotes.

@microsoft-github-policy-service microsoft-github-policy-service bot removed the Help-Wanted The issue is up-for-grabs, and can be claimed by commenting label Jun 10, 2024
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.

4 participants