Skip to content

Add getter-only configuration binding parity tests - #131593

Open
svick wants to merge 3 commits into
dotnet:mainfrom
svick:add-binder-get-only-property-tests
Open

Add getter-only configuration binding parity tests#131593
svick wants to merge 3 commits into
dotnet:mainfrom
svick:add-binder-get-only-property-tests

Conversation

@svick

@svick svick commented Jul 30, 2026

Copy link
Copy Markdown
Member

Adds the shared reflection/source-generator parity tests requested in this review comment: #131045 (review).

The tests cover get-only nested objects, collections, and abstract-typed properties when their existing values are both non-null and null. They verify that existing instances are bound and null properties are ignored without throwing.

Note

This pull request was created with GitHub Copilot.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @dotnet/area-extensions-configuration
See info in area-owners.md if you want to be subscribed.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds new xUnit coverage to ensure configuration binding behavior for getter-only properties stays consistent (and non-throwing) across the binder implementations, focusing on binding into existing instances vs ignoring null getter-only properties.

Changes:

  • Added a new test model type with getter-only nested/collection/abstract-typed properties that can be pre-initialized or left null.
  • Added tests verifying binding mutates existing non-null instances and ignores null getter-only properties without throwing.

Reviewed changes

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

File Description
src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.TestClasses.cs Adds ClassWithGetterOnlyProperties test type used to exercise getter-only binding scenarios.
src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.cs Adds two new [Fact] tests validating binding behavior for getter-only properties when values are initialized vs null.

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings July 30, 2026 14:58

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 2 out of 2 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (3)

src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.TestClasses.cs:1097

  • To make the null-values case a stronger regression test for “skip binding when getter-only property value is null”, it helps if the collection element type can actually fail to bind. With List<string>, virtually any configuration value binds successfully, so a regression that allocates/binds a temporary list (then discards it) would still pass. Consider switching this to List<int> so the test can use non-parsable values and fail if binding is attempted.
            public NestedOptions? Nested { get; }
            public List<string>? Collection { get; }
            public AbstractBase? Abstract { get; }

src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.cs:2965

  • If Collection is switched to List<int>, update the non-null case JSON and assertion accordingly; this keeps the test validating that binding into an existing getter-only collection instance actually happened (and didn’t just no-op).
                {
                    "Nested": { "Integer": 1 },
                    "Collection": [ "item" ],
                    "Abstract": { "Value": 2 }
                }

src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.cs:2978

  • The null-values test currently uses bindable values for Nested and Collection, so a regression that still tries to bind a temporary instance for null getter-only properties (then discards it) could pass without being detected. Consider using deliberately non-parsable values so the test fails if binding is attempted (this becomes especially effective once Collection is List<int>).
            IConfiguration configuration = TestHelpers.GetConfigurationFromJsonString(
                """
                {
                    "Nested": { "Integer": 1 },
                    "Collection": [ "item" ],
                    "Abstract": { "Value": 2 }
                }
                """);

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings July 31, 2026 09:26

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 2 out of 2 changed files in this pull request and generated no new comments.

@svick
svick marked this pull request as ready for review July 31, 2026 13:07
@svick
svick requested review from mrek-msft, rosebyte and tarekgh July 31, 2026 13:08
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants