Skip to content

Handle illegal xml char #98

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

Closed

Conversation

Siphonophora
Copy link
Contributor

@Siphonophora Siphonophora commented Mar 18, 2023

Depends on spekt/testlogger#37 to be merged then we can use the new package

@Siphonophora
Copy link
Contributor Author

@codito In NUnitXmlSeralizer, I am not sure what to do with these two methods below. They both have code that is specific to NUnit framework and they are the only place we are using TestCase.Properties.

  • CreateSeedAttribute
  • CreatePropertiesElement - uses GetPropertyValue which looks like it might be tricky to replicate if we tried to follow the same approach of sanitizing all data before loading into TestResultInfo.

@@ -242,7 +244,7 @@ private static XElement CreateTestCaseElement(TestResultInfo result)
{
var element = new XElement(
"test-case",
new XAttribute("name", result.TestCase.DisplayName),
new XAttribute("name", result.Method),
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should use result.TestCaseDisplayName to avoid regressions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we use the display name, we are giving up the benefit of the adapter specific code in the core logger. At is stands, only the junit logger is using the method right now. But I thought that was the goal of the core logger.

I agree this could be a breaking change, but it seems like the right direction.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, maybe this issue is going to be important. spekt/junit.testlogger#57

<PackageReference Include="System.ValueTuple" Version="4.3.0" />
</ItemGroup>

<ItemGroup>
Copy link
Collaborator

Choose a reason for hiding this comment

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

We may need to undo this for the CI to pass.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I will. Its just for my dev right now

@codito
Copy link
Collaborator

codito commented Mar 19, 2023

CreateSeedAttribute doesn't have user inputs. It is okay to not sanitize it.

CreatePropertiesElement will need sanitization. Let's use InputSanitizer.Sanitize directly here?

@codito codito marked this pull request as draft July 3, 2023 10:47
@codito codito marked this pull request as ready for review July 4, 2023 17:22
@codito
Copy link
Collaborator

codito commented Jul 4, 2023

Open items:

(1) An alternative to exposing TestProperty directly is to fix spekt/testlogger#17 via the adapter specific extensions in core testlogger. We need to resolve spekt/testlogger#17 before merging this.

(2) TestCaseDisplayName and MethodFormat extensibility should be in core testlogger?

@codito codito marked this pull request as draft July 5, 2023 05:09
@codito codito mentioned this pull request Jul 5, 2023
@codito
Copy link
Collaborator

codito commented Jul 5, 2023

Continued in #100

@codito codito closed this Jul 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants