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.

Sign up for GitHub

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

Jump to bottom

Tuple missing types #65

Merged
merged 6 commits into from
Oct 10, 2020
Merged

Tuple missing types #65

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
194712d
Test cases covers tuple issue
KonH Jul 7, 2020
da985a1
Proposed solution to track extra parentheses
KonH Jul 7, 2020
63f2a07
Add more complex tuple test cases
KonH Sep 24, 2020
6a5e483
Add more test cases for non-parsable inputs
KonH Sep 25, 2020
402528c
Closing parenthesis should be treated as error only when it is not pa…
KonH Sep 25, 2020
141e96c
Treat backslashes as errors only if they not inside parenthesis block
KonH Sep 27, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 19 additions & 3 deletions src/NUnit.Xml.TestLogger/TestCaseNameParser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ public static ParsedName Parse(string fullyQualifedName)

var step = NameParseStep.FindMethod;
var state = NameParseState.Default;
var parenthesisCount = 0;

var output = new List<char>();

Expand All @@ -71,13 +72,23 @@ public static ParsedName Parse(string fullyQualifedName)
}
else if (state == NameParseState.Default)
{
if (thisChar == '(' || thisChar == '"' || thisChar == '\\')
if (thisChar == '(')
{
parenthesisCount--;
}

// Backslashes allowed only inside parenthesis block
if ((thisChar == '\\') && (parenthesisCount == 0))
{
throw new Exception("Found invalid characters");
}
else if (thisChar == '"')
{
throw new Exception("Found invalid characters");
}
else if (thisChar == ')')
{
if (output.Count > 0)
if ((output.Count > 0) && (parenthesisCount == 0))
{
throw new Exception("The closing parenthesis we detected wouldn't be the last character in the output string. This isn't acceptable because we aren't in a string");
}
Expand Down Expand Up @@ -125,7 +136,7 @@ public static ParsedName Parse(string fullyQualifedName)
{
if (thisChar == ')')
{
throw new Exception("Found invalid characters");
parenthesisCount++;
}

if (thisChar == '(')
Expand Down Expand Up @@ -157,6 +168,11 @@ public static ParsedName Parse(string fullyQualifedName)
}
}

if (parenthesisCount != 0)
{
throw new Exception($"Unbalanced count of parentheses found ({parenthesisCount})");
}

// We are done. If we are finding type, set that variable.
// Otherwise, ther was some issue, so leave the type blank.
if (step == NameParseStep.FindNamespace)
Expand Down
10 changes: 10 additions & 0 deletions test/NUnit.Xml.TestLogger.UnitTests/TestCaseNameParserTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,12 @@ public class TestCaseNameParserTests

// See nunit.testlogger #66.
[DataRow("z.y.x.ape.bar(a\\b)", "z.y.x", "ape", "bar(a\\b)")]

// Test with tuple arguments
[DataRow("z.a.b((0,1))", "z", "a", "b((0,1))")]
[DataRow("z.a.b((\"arg\",1))", "z", "a", "b((\"arg\",1))")]
Copy link
Contributor

Choose a reason for hiding this comment

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

@KonH Thanks for pointing this out.

Sorry for the delay in reviewing this. Here are some more complex test cases which currently fail. If you want to work on fixing this let me know. Otherwise, I can take more of a look in a few days.

    [DataRow("z.a.b((0,1),(2,3))", "z", "a", "b((0,1),(2,3))")]
    [DataRow("z.a.b((0,(0,1)),(0,1))", "z", "a", "b((0,(0,1)),(0,1))")]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I want, will be trying to fix it soon

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good, Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add more test cases and push solution for them, now tests passed
I'm not sure about other corner cases, but that approach looks suitable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But appveyor build failed for some reason (as you can see, dependencies aren't changed in that branch):
C:\Program Files\dotnet\sdk\3.1.401\Sdks\NuGet.Build.Tasks.Pack\build\NuGet.Build.Tasks.Pack.targets(198,5): error NU5104: A stable release of a package should not have a prerelease dependency. Either modify the version spec of dependency "Stylecop.Analyzers [1.1.0-beta009, )" or update the version field in the nuspec. [C:\projects\nunit-testlogger\src\NUnit.Xml.TestLogger\NUnit.Xml.TestLogger.csproj]

Copy link
Contributor

Choose a reason for hiding this comment

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

@KonH Thanks. I'm happy with the changes. The appveyor issue is from our tooling. I think you won't need to make changes for that.

@codito I think all 3 pulls are ready to be merged. #68 Adds the logo and would resolve all the appveyor issues. If we subsequently merge this and #67 then Master shouldn't have build issues.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, taking a look and #65 and #67 made some changes which aren't totally compatible. Let me sort that out

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, so it was a simple fix when pulling it into the junit logger. spekt/junit.testlogger@94af6c5

[DataRow("z.a.b((0,1),(2,3))", "z", "a", "b((0,1),(2,3))")]
[DataRow("z.a.b((0,(0,1)),(0,1))", "z", "a", "b((0,(0,1)),(0,1))")]
public void Parse_ParsesAllParsableInputs_WithoutConsoleOutput(string testCaseName, string expectedNamespace, string expectedType, string expectedMethod)
{
var expected = new Tuple<string, string>(expectedType, expectedMethod);
Expand Down Expand Up @@ -94,6 +100,10 @@ public void Parse_ParsesAllParsableInputsWithoutNamespace_WithConsoleOutput(stri
[DataRow("z.y.x.")]
[DataRow("z.y.x.)")]
[DataRow("z.y.x.\"\")")]
[DataRow("z.a.b((0,1)")]
[DataRow("z.a.b((0,1)))")]
[DataRow("z.a.b((0,(0,1))")]
[DataRow("z.a.b((0,(0,1))))")]
public void Parse_FailsGracefullyOnNonParsableInputs_WithConsoleOutput(string testCaseName)
{
var expectedConsole = string.Format(
Expand Down