Skip to content

Make DisplayOptions JSON-deserializable#153

Merged
boxofyellow merged 10 commits into
mainfrom
copilot/expand-test-with-optional-resource
May 22, 2026
Merged

Make DisplayOptions JSON-deserializable#153
boxofyellow merged 10 commits into
mainfrom
copilot/expand-test-with-optional-resource

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented May 18, 2026

Code-coverage reports flagged uncovered branches across the new STJ converters and DisplayOptions (de)serialization helpers introduced for JSON round-tripping. This PR closes those gaps.

  • JsonConverterCoverageTests.cs (new, 12 tests)

    • HeaderStyleJsonConverter: missing / null / unknown $type discriminator (read), unsupported IHeaderStyle impl (write).
    • TextColorJsonConverter: RGB-channel round-trip and missing-channel fallback to zero.
    • DisplayOptions.DeserializeAsync(Stream): round-trip with and without caller-supplied JsonSerializerOptions; also exercises the Header is FigletTextStyle arm in EnsureHeaderFontsLoadedAsync.
    • JsonWriteHelpers.ShouldIgnore: Always arm and out-of-range default throw (unreachable via the public JsonSerializerOptions.DefaultIgnoreCondition setter, which rejects Always at runtime).
  • Dead-code removal in converter Read methods

    • Removed if (reader.TokenType == JsonTokenType.Null) return null; from both HeaderStyleJsonConverter and TextColorJsonConverter. System.Text.Json short-circuits null tokens for reference-type converters that don't override HandleNull, so those branches were never invoked. JSON null for Header / Foreground still deserializes to null via the framework path (asserted by tests).
  • JsonWriteHelpers.ShouldIgnore<T> visibility

    • Promoted from private to internal so tests can invoke it directly (the test project already has InternalsVisibleTo).

Result: 0 uncovered lines remain in DisplayOptions.cs, HeaderStyleJsonConverter.cs, TextColorJsonConverter.cs, and JsonWriteHelpers.cs.

Co-authored-by: boxofyellow <54955040+boxofyellow@users.noreply.github.com>
Copy link
Copy Markdown
Owner

@boxofyellow boxofyellow left a comment

Choose a reason for hiding this comment

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

@copilot please review these comments.

Comment thread ConsoleMarkdownRenderer.Tests/RendererTests.cs Outdated
Comment thread ConsoleMarkdownRenderer.Tests/RendererTests.cs Outdated
Comment thread Styling/TextColor.cs Outdated
Comment thread Styling/FigletTextStyle.cs Outdated
…sync, internal converters

Co-authored-by: boxofyellow <54955040+boxofyellow@users.noreply.github.com>
Copilot AI requested a review from boxofyellow May 18, 2026 11:42
Copy link
Copy Markdown
Owner

@boxofyellow boxofyellow left a comment

Choose a reason for hiding this comment

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

@copilot Please checkout these comments.

Comment thread Styling/FigletTextStyle.cs Outdated
Comment thread DisplayOptions.cs Outdated
Comment thread DisplayOptions.cs Outdated
Comment thread Styling/HeaderStyleJsonConverter.cs
Comment thread Styling/HeaderStyleJsonConverter.cs Outdated
Comment thread ConsoleMarkdownRenderer.Tests/RendererTests.cs
…self-sufficient converters; convert tests

Co-authored-by: boxofyellow <54955040+boxofyellow@users.noreply.github.com>
Copilot AI requested a review from boxofyellow May 20, 2026 01:27
Copy link
Copy Markdown
Owner

@boxofyellow boxofyellow left a comment

Choose a reason for hiding this comment

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

@copilot Please check out these comments.

Comment thread ConsoleMarkdownRenderer.Tests/resources/emojiDisabled.json Outdated
Comment thread ConsoleMarkdownRenderer.Tests/FigletTextStyleTests.cs Outdated
Comment thread ConsoleMarkdownRenderer.Tests/FigletTextStyleTests.cs Outdated
Comment thread Styling/HeaderStyleJsonConverter.cs Outdated
Comment thread Styling/HeaderStyleJsonConverter.cs Outdated
Comment thread Styling/TextColorJsonConverter.cs Outdated
Comment thread Styling/JsonEnumHelpers.cs Outdated
…or, camelCase, options-aware enum (de)serialization

Co-authored-by: boxofyellow <54955040+boxofyellow@users.noreply.github.com>
Copilot AI requested a review from boxofyellow May 20, 2026 11:49
Copy link
Copy Markdown
Owner

@boxofyellow boxofyellow left a comment

Choose a reason for hiding this comment

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

@copilot Please check out this comment.

Comment thread Styling/HeaderStyleJsonConverter.cs Outdated
…yling JSON converters

Co-authored-by: boxofyellow <54955040+boxofyellow@users.noreply.github.com>
Copilot AI requested a review from boxofyellow May 21, 2026 11:21
Copy link
Copy Markdown
Owner

@boxofyellow boxofyellow left a comment

Choose a reason for hiding this comment

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

@copilot Please address this comment.

Comment thread Styling/HeaderStyleJsonConverter.cs Outdated
Co-authored-by: boxofyellow <54955040+boxofyellow@users.noreply.github.com>
Copilot AI requested a review from boxofyellow May 21, 2026 11:38
@boxofyellow boxofyellow changed the title Make DisplayOptions JSON-deserializable via async-lazy FigletTextStyle font loading Make DisplayOptions JSON-deserializable May 21, 2026
Comment thread docs/CHANGELOG.md Outdated
Comment thread Styling/JsonWriteHelpers.cs Outdated
Comment thread DisplayOptions.cs
Co-authored-by: boxofyellow <54955040+boxofyellow@users.noreply.github.com>
Comment thread Styling/JsonWriteHelpers.cs Outdated
Comment thread Styling/JsonWriteHelpers.cs Outdated
Fix build

Co-authored-by: boxofyellow <54955040+boxofyellow@users.noreply.github.com>
Copy link
Copy Markdown
Owner

@boxofyellow boxofyellow left a comment

Choose a reason for hiding this comment

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

@copilot there are some build breaks I would you take a look at

https://github.com/boxofyellow/ConsoleMarkdownRenderer/actions/runs/26260426954/job/77292440234

But there is also

https://github.com/boxofyellow/ConsoleMarkdownRenderer/actions/runs/26260426954/job/77292440220

For net8.0 only failure I think we need to make those cases conditionally compiled.

Co-authored-by: boxofyellow <54955040+boxofyellow@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented May 22, 2026

@copilot there are some build breaks I would you take a look at

https://github.com/boxofyellow/ConsoleMarkdownRenderer/actions/runs/26260426954/job/77292440234...

Fixed in 29831c6 — corrected the JsonIgnorecondition typo and wrapped the WhenWriting / WhenReading arms in #if NET10_0_OR_GREATER since those enum members don't exist in net8.0. Verified dotnet build for both net8.0 and net10.0, and all 175 tests pass on net8.0.

Copilot AI requested a review from boxofyellow May 22, 2026 00:13
@codecov
Copy link
Copy Markdown

codecov Bot commented May 22, 2026

Codecov Report

❌ Patch coverage is 97.63033% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 96.09%. Comparing base (bb164d3) to head (5b279b0).

Files with missing lines Patch % Lines
DisplayOptions.cs 96.92% 0 Missing and 2 partials ⚠️
Styling/HeaderStyleJsonConverter.cs 97.26% 0 Missing and 2 partials ⚠️
Styling/TextColorJsonConverter.cs 97.29% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #153      +/-   ##
==========================================
+ Coverage   95.92%   96.09%   +0.16%     
==========================================
  Files          31       34       +3     
  Lines        1743     1946     +203     
  Branches      182      214      +32     
==========================================
+ Hits         1672     1870     +198     
  Misses         46       46              
- Partials       25       30       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Owner

@boxofyellow boxofyellow left a comment

Choose a reason for hiding this comment

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

@copilot I listed all the lines that code coverage workflow says were not covered.

Before doing any of them please double check that the workflow is set up correctly
https://github.com/boxofyellow/ConsoleMarkdownRenderer/blob/main/.github/workflows/code-coverage.yml

Comment thread Styling/HeaderStyleJsonConverter.cs Outdated
Comment thread Styling/HeaderStyleJsonConverter.cs
Comment thread Styling/HeaderStyleJsonConverter.cs
Comment thread Styling/HeaderStyleJsonConverter.cs
Comment thread Styling/HeaderStyleJsonConverter.cs
Comment thread Styling/TextColorJsonConverter.cs
Comment thread DisplayOptions.cs
Comment thread DisplayOptions.cs
Comment thread Styling/JsonWriteHelpers.cs
Comment thread Styling/JsonWriteHelpers.cs
Copy link
Copy Markdown
Contributor Author

Copilot AI commented May 22, 2026

@copilot I listed all the lines that code coverage workflow says were not covered.

Before doing any of them please double check that the workflow is set up correctly...

Walked through .github/workflows/code-coverage.yml and reproduced the runs locally — the workflow looks correctly set up:

  • dotnet test ConsoleMarkdownRenderer.Tests /p:CollectCoverage=true /p:CoverletOutputFormat=opencover (without --framework) runs once per TFM, producing coverage.net8.0.opencover.xml and coverage.net10.0.opencover.xml in the test project directory. Same shape for ConsoleMarkdownRenderer.ExampleTests. All four files exist after the run.
  • codecov-action is configured with use_oidc: true and no explicit files:, so it auto-discovers all four reports — which is the documented happy-path for multi-TFM coverlet output.
  • Locally on the current branch I reproduced the same picture codecov sees: the main library shows 96.21% line / 87.39% branch coverage, and the specific lines you flagged are genuinely uncovered on both TFMs (e.g. DisplayOptions.cs 318–324 is the Stream DeserializeAsync overload — never invoked by any test; 336–338 is the options.Header is FigletTextStyle branch — no fixture sets a Figlet header; the throw/null-path arms in the converters likewise have no negative-path tests yet).

One thing worth calling out about your DisplayOptions.cs:292 comment: line 292 on the current branch is the first line of the string DeserializeAsync (ArgumentNullException.ThrowIfNull(json);), which my local report shows hit 7 times on both net8.0 and net10.0. The fully-uncovered sibling is the stream DeserializeAsync (lines 314–324). Codecov may have grouped the two method definitions together visually, which is probably what's making it look like "none of this method" is covered. Either way the underlying gap (stream overload + Header-as-Figlet branch) is real.

So: no workflow fix needed. Ready to go fill in the missing tests — want me to proceed on all of the line-level comments?

Co-authored-by: boxofyellow <54955040+boxofyellow@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented May 22, 2026

@copilot I listed all the lines that code coverage workflow says were not covered.

Before doing any of them please double check that the workflow is set up correctly...

Added coverage in JsonConverterCoverageTests.cs (12 new tests). All four flagged files now report 0 uncovered lines on both net8.0 and net10.0; 187/187 tests pass.

Notes:

  • For the two if (reader.TokenType == JsonTokenType.Null) return null; branches in HeaderStyleJsonConverter.Read and TextColorJsonConverter.Read: those were dead — System.Text.Json short-circuits null tokens for reference-type targets without invoking the converter (since neither overrides HandleNull to true). Removed them. Tests still assert "header": null and "foreground": null round-trip to null via the framework path.
  • JsonWriteHelpers.ShouldIgnore was promoted from private to internal so the Always arm (unreachable via the public DefaultIgnoreCondition setter, which rejects Always at runtime) and the out-of-range default throw could be exercised directly. InternalsVisibleTo was already in place.
  • Stream DeserializeAsync and the Header is FigletTextStyle branch in EnsureHeaderFontsLoadedAsync are now covered by two stream-based round-trip tests.

Copilot AI changed the title Make DisplayOptions JSON-deserializable Cover JSON converter edge cases in DisplayOptions serialization May 22, 2026
@boxofyellow boxofyellow marked this pull request as ready for review May 22, 2026 00:58
@boxofyellow boxofyellow changed the title Cover JSON converter edge cases in DisplayOptions serialization Make DisplayOptions JSON-deserializable May 22, 2026
@boxofyellow boxofyellow merged commit 3777410 into main May 22, 2026
12 checks passed
@boxofyellow boxofyellow deleted the copilot/expand-test-with-optional-resource branch May 22, 2026 00:59
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