Improve code coverage for System.Security.Cryptography.Xml #107235
Conversation
Fixes dotnet#20508 Add tests for `KeyInfoClause` and `CanonicalXmlEntityReference` to improve code coverage. * **KeyInfoClauseTests.cs** - Add a new test class `KeyInfoClauseTests`. - Add a test method `LoadXml_ValidXml_Success` to test loading valid XML. - Add a test method `LoadXml_InvalidXml_ThrowsException` to test loading invalid XML. - Add a test method `GetXml_ReturnsExpectedXml` to test getting XML from `KeyInfoClause`. * **CanonicalXmlEntityReferenceTests.cs** - Add a new test class `CanonicalXmlEntityReferenceTests`. - Add a test method `Write_WritesExpectedOutput` to test writing canonical XML. - Add a test method `WriteHash_WritesExpectedHash` to test writing hash of canonical XML. - Add a test method `IsInNodeSet_GetSet_ReturnsExpected` to test getting and setting `IsInNodeSet`.
ghost
added
the
dotnet-policy-service
bot
added
the
community-contribution
There was a problem hiding this comment.
👋 thanks for the contribution!
The dotnet/runtime repository does not automatically include .cs files in projects, they must be explicitly added to the project file.
As a result, these tests are not actually running. Can you please add them to https://github.com/dotnet/runtime/blob/main/src/libraries/System.Security.Cryptography.Xml/tests/System.Security.Cryptography.Xml.Tests.csproj?
src/libraries/System.Security.Cryptography.Xml/tests/CanonicalXmlEntityReferenceTests.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Security.Cryptography.Xml/tests/CanonicalXmlEntityReferenceTests.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Security.Cryptography.Xml/tests/CanonicalXmlEntityReferenceTests.cs
Outdated
Show resolved
Hide resolved
| { | ||
| var xmlDocument = new XmlDocument(); | ||
| xmlDocument.LoadXml("<KeyInfo><KeyName>Test</KeyName></KeyInfo>"); | ||
| var keyInfoClause = new TestKeyInfoClause(); |
There was a problem hiding this comment.
It's not very clear to me what we are testing here. All of these tests are testing a test class, so the tests are just testing TestKeyInfoClause.
The only thing on KeyInfoClause that actually has functionality to be tested is GetXml(XmlDocument xmlDocument) when it isn't overridden.
… bool verifySignatureOnly)`, `System.Security.Cryptography.Xml.KeyInfoClause`, and `System.Security.Cryptography.Xml.CanonicalXmlEntityReference` in `EncryptedXmlTest.cs`. * **CheckSignature**: Add test for `CheckSignature` method with `X509Certificate2` and `verifySignatureOnly` parameter. * **KeyInfoClause**: Add tests for `GetXml` and `LoadXml` methods. * **CanonicalXmlEntityReference**: Add tests for `Write`, `WriteHash`, and `IsInNodeSet` methods. Add `SymmetricKeyWrapTest.cs` to test `System.Security.Cryptography.Xml.SymmetricKeyWrap`. Add `EncryptionMethodTest.cs` to test `System.Security.Cryptography.Xml.EncryptionMethod`.
Add tests for `SignedXml.CheckSignature` base on code review
Two new test files, `CanonicalXmlEntityReferenceTests.cs` and `KeyInfoClauseTests.cs`, have been added to the project file `System.Security.Cryptography.Xml.Tests.csproj`. These files are included in the `<ItemGroup>` section, which lists the source files to be compiled.
Modified access modifiers of three test methods in `CanonicalXmlEntityReferenceTests.cs`: - `Write_WritesExpectedOutput` - `WriteHash_WritesExpectedHash` - `IsInNodeSet_GetSet_ReturnsExpected` Changed from instance methods (`public void`) to static methods (`public static void`).
Refactored `CanonicalXmlEntityReferenceTests.cs` and `KeyInfoClauseTests.cs` to replace explicit type declarations with `var` where the type is evident from the context. This change improves code readability and conciseness without altering functionality. Specifically, updated `XmlDocument`, `CanonicalXmlEntityReference`, `KeyInfoName`, and `XmlElement` variables.
src/libraries/System.Security.Cryptography.Xml/tests/EncryptionMethodTest.cs
Show resolved
Hide resolved
...braries/System.Security.Cryptography.Xml/tests/System.Security.Cryptography.Xml.Tests.csproj
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
As the tests are now showing, these tests do not compile because you are attempting to test and use internal types and members.
The unit test project here does not have access to internal members, and we very rarely make use of InternalsVisibleTo. internal things are typically tested by way of them being used from public things (behavioral tests).
I would also consider running tests locally before you submit them, or at least ensure they compile. The documentation on how to get started with local developer is in the workflow instructions.
At this point I would recommend identifying public types and functionality that are lacking test coverage.
| public static void Write_WritesExpectedOutput() | ||
| { | ||
| var xmlDocument = new XmlDocument(); | ||
| var entityReference = new CanonicalXmlEntityReference("entity", xmlDocument, true); |
There was a problem hiding this comment.
The tests are failing to compile now because CanonicalXmlEntityReference is internal. We don't typically unit test internal classes, only the public API surface.
| var xmlDocument = new XmlDocument(); | ||
| var entityReference = new CanonicalXmlEntityReference("entity", xmlDocument, true); | ||
| var stringBuilder = new StringBuilder(); | ||
| var anc = new AncestralNamespaceContextManager(); |
There was a problem hiding this comment.
Likewise, this type is internal.
vcsjones
added
the
needs-author-action
Fixes #20508
Add tests for
KeyInfoClauseandCanonicalXmlEntityReferenceto improve code coverage.KeyInfoClauseTests.cs
KeyInfoClauseTests.LoadXml_ValidXml_Successto test loading valid XML.LoadXml_InvalidXml_ThrowsExceptionto test loading invalid XML.GetXml_ReturnsExpectedXmlto test getting XML fromKeyInfoClause.CanonicalXmlEntityReferenceTests.cs
CanonicalXmlEntityReferenceTests.Write_WritesExpectedOutputto test writing canonical XML.WriteHash_WritesExpectedHashto test writing hash of canonical XML.IsInNodeSet_GetSet_ReturnsExpectedto test getting and settingIsInNodeSet.