Skip to content

Conversation

@AaronRobinsonMSFT
Copy link
Member

@AaronRobinsonMSFT AaronRobinsonMSFT commented Jan 22, 2026

Using Visual Studio's Code Coverage tool, reported code coverage is 71.7 %.

Created test suite for the DirectWriteForwarder project:

Test coverage includes:

  • Font, FontFace, FontFamily, FontCollection classes
  • All enums and structs (FontWeight, FontStretch, FontStyle, etc.)
  • FontMetrics and GlyphMetrics calculations
  • DWriteMatrix transforms (identity, scale, rotation)
  • LocalizedStrings IDictionary implementation
  • FontFile, FontSource, FontSourceFactory infrastructure
  • FontFileLoader, FontFileStream, FontFileEnumerator
  • ItemProps, Span, and ClassificationUtility for text analysis
  • TrueTypeSubsetter.ComputeSubset with unsafe code
  • Unsafe pointer APIs (TryGetFontTable, GetGlyphIndices, GetGlyphMetrics)
  • TextAnalyzer integration tests (Itemize, GetGlyphs, GetGlyphPlacements)
  • Edge cases and error handling (boundary conditions, invalid inputs)

Infrastructure changes:

  • Added InternalsVisibleTo for DirectWriteForwarder.Tests in DirectWriteForwarder
  • Added InternalsVisibleTo for DirectWriteForwarder.Tests in PresentationCore
  • Created test project targeting net10.0-windows with xUnit and FluentAssertions
Microsoft Reviewers: Open in CodeFlow

Created comprehensive test suite for the DirectWriteForwarder project:

Test coverage includes:
- Font, FontFace, FontFamily, FontCollection classes
- All enums and structs (FontWeight, FontStretch, FontStyle, etc.)
- FontMetrics and GlyphMetrics calculations
- DWriteMatrix transforms (identity, scale, rotation)
- LocalizedStrings IDictionary implementation
- FontFile, FontSource, FontSourceFactory infrastructure
- FontFileLoader, FontFileStream, FontFileEnumerator
- ItemProps, Span, and ClassificationUtility for text analysis
- TrueTypeSubsetter.ComputeSubset with unsafe code
- Unsafe pointer APIs (TryGetFontTable, GetGlyphIndices, GetGlyphMetrics)
- TextAnalyzer integration tests (Itemize, GetGlyphs, GetGlyphPlacements)
- Edge cases and error handling (boundary conditions, invalid inputs)

Infrastructure changes:
- Added InternalsVisibleTo for DirectWriteForwarder.Tests in DirectWriteForwarder
- Added InternalsVisibleTo for DirectWriteForwarder.Tests in PresentationCore
- Created test project targeting net10.0-windows with xUnit and FluentAssertions
@AaronRobinsonMSFT AaronRobinsonMSFT requested review from a team, JeremyKuhne and Copilot and removed request for Copilot January 22, 2026 18:56
@dotnet-policy-service dotnet-policy-service bot added the PR metadata: Label to tag PRs, to facilitate with triage label Jan 22, 2026
- Add TestHelpers.cs with centralized skip helpers using xUnit v3 Assert.SkipUnless()
- Add AdditionalCoverageTests.cs for LocalizedStrings, FontList, Font, FontFace
- Add DWriteTypeConverterTests.cs with direct Convert() method calls (139 tests)
- Add TextAnalyzerControlCharacterTests.cs for control chars, hyphens, zero-width
- Add TtfDeltaTests.cs for GlobalInit.Init() and ControlTableInit.Init() coverage
- Update existing tests to use Assert.SkipUnless() instead of early returns
- Tests now report 'skipped' when fonts unavailable instead of falsely 'passed'

Total: 577 tests passing, coverage improved from 17% to 58%
Copilot AI review requested due to automatic review settings January 23, 2026 00:41
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This pull request adds comprehensive unit tests for the DirectWriteForwarder project in WPF. The test suite covers font rendering and text analysis functionality using DirectWrite APIs.

Changes:

  • Created a new test project with xUnit and FluentAssertions
  • Added InternalsVisibleTo attributes to expose internal types for testing
  • Implemented extensive test coverage for Font, FontFace, FontFamily, FontCollection, TextAnalyzer, and supporting types

Reviewed changes

Copilot reviewed 21 out of 21 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
DirectWriteForwarder.Tests.csproj Test project configuration with dependencies
OtherAssemblyAttrs.cs/cpp InternalsVisibleTo attributes for test access
TestHelpers.cs Common test utilities and helper methods
Font/FontFace/FontCollection tests Core font API testing
TextAnalyzerIntegrationTests.cs Integration tests for text analysis
EnumTests.cs Enum value validation tests
EdgeCaseTests.cs Boundary condition and error handling tests
DWriteTypeConverterTests.cs Type conversion testing
TrueTypeSubsetterTests.cs Font subsetting functionality tests
TtfDeltaTests.cs TrueType delta functionality tests

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Add 35 new tests in TtfDeltaCoverageTests.cs to improve TrueType subsetter coverage:
- TTC (TrueType Collection) handling with proper font offsets
- CJK fonts (SimSun, MS Gothic, Malgun)
- Symbol fonts (Symbol, Wingdings, Webdings, Segoe UI Symbol)
- Emoji fonts (Segoe UI Emoji)
- Various font types (Segoe UI, Gabriola, Times, Courier, etc.)
- Different glyph patterns (sparse, consecutive, boundary values)

Also fix TrueTypeSubsetterTests.cs to avoid using fixed pointers
in lambda expressions (CS1764 compiler error).

Test count: 577 -> 612
Line coverage: 58.8% -> 71.7%
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 22 out of 22 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@AaronRobinsonMSFT
Copy link
Member Author

I'm currently done with this PR. The highlevel is the tests appear to be passing on all legs, look for DirectWriteForwarder.Tests, the reported code coverage is 71.1 %, and the only product change is adding the InternalsVisibleTo attribute to the DirectWriteForwarder and PresentationCore for the DirectWriteForwarder.Tests project.

/cc @dipeshmsft @JeremyKuhne @agocke @jkoritzinsky

JeremyKuhne
JeremyKuhne previously approved these changes Jan 23, 2026
Copy link
Member

@jkoritzinsky jkoritzinsky left a comment

Choose a reason for hiding this comment

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

Not going to comment throughout, but a lot of the tests (especially in DWriteTypeConverterTests.cs) just validate that a given API doesn't throw. There's minimal validation of the values returned from the APIs.

As a result, this testing doesn't feel particularly meaningful. I think (especially for the font tests) we should have a font file that is used as a well-known input so the tests can actually validate against the outputs of the APIs, not just assume that "any result" is the correct result.

@ThomasGoulet73
Copy link
Contributor

@AaronRobinsonMSFT I'm a bit curious with the background for this PR, is it because you're planning on working on DirectWriteForwarder or is it just to increase code coverage (Maybe to increase testing of C++/CLI for the .Net interop team) ?

I ask this because I have open PRs (#9902 and #10523, though they've been open for a while) and more code locally to migrate DirectWriteForwarder to C# and I just want to make sure I'm not stepping on anybody's toes.

In any case, this PR would help a lot with migrating DirectWriteForwarder to C# so that's very nice.

@AaronRobinsonMSFT
Copy link
Member Author

more code locally to migrate DirectWriteForwarder to C#

@ThomasGoulet73 This is indeed the long term goal, but we need tests. That is the purpose/beginning of this PR process. We need measurable metrics in order to have confidence in any port to C# and the cost of doing it manually is non-zero. The current suite of WPF integration and unit tests is helpful but not sufficient for something as low level and sensitive as the DirectWriteForwarder assembly. Regardless if/when moving toward C# or not, the tests help us have confidence in changes so that is a win I'm assuming we can all agree on.

In any case, this PR would help a lot with migrating DirectWriteForwarder to C# so that's very nice.

Yep.

@ThomasGoulet73
Copy link
Contributor

This is indeed the long term goal, but we need tests. That is the purpose/beginning of this PR process. We need measurable metrics in order to have confidence in any port to C# and the cost of doing it manually is non-zero. The current suite of WPF integration and unit tests is helpful but not sufficient for something as low level and sensitive as the DirectWriteForwarder assembly. Regardless if/when moving toward C# or not, the tests help us have confidence in changes so that is a win I'm assuming we can all agree on.

That's good news, thanks

- Extend system font iteration to all families (removed 50-family limit)
- Add value verification for native->managed type conversions
- Add TrueType structure validation (signature, tables, offsets)
- Add known Arial reference values for Font/FontFace properties
- Enhance FontFaceType test with TTC and OTF file checks
- Fix misleading comments in font style and subset tests
Validate known Arial font strings:
- CopyrightNotice contains 'Monotype'
- Trademark contains 'Arial'
- Manufacturer contains 'Monotype'
- WIN32FamilyNames contains 'Arial'
- Win32SubFamilyNames contains 'Normal'
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 23 out of 23 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

jkoritzinsky
jkoritzinsky previously approved these changes Jan 27, 2026
Copy link
Member

@jkoritzinsky jkoritzinsky left a comment

Choose a reason for hiding this comment

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

Looks fine enough for the goal of this PR.

Would like to see the comment from the Copilot review applied before this is merged though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

PR metadata: Label to tag PRs, to facilitate with triage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants