Skip to content

Add regression test for #34713: Binding with Converter and x:DataType is compiled#34717

Merged
StephaneDelcroix merged 1 commit intomainfrom
34713-07fa
Mar 28, 2026
Merged

Add regression test for #34713: Binding with Converter and x:DataType is compiled#34717
StephaneDelcroix merged 1 commit intomainfrom
34713-07fa

Conversation

@StephaneDelcroix
Copy link
Copy Markdown
Contributor

@StephaneDelcroix StephaneDelcroix commented Mar 28, 2026

Description

Adds regression tests for #34713 verifying the XAML source generator correctly handles bindings with Converter={StaticResource ...} inside x:DataType scopes.

Closes #34713

Investigation

After thorough investigation of the source generator pipeline (KnownMarkups.cs, CompiledBindingMarkup.cs, NodeSGExtensions.cs):

When converter IS in page resources (compile-time resolution ✅)

GetResourceNode() walks the XAML tree, finds the converter resource, and ProvideValueForStaticResourceExtension returns the variable directly — no runtime ProvideValue call. The converter is referenced at compile time.

When converter is NOT in page resources (runtime resolution ✅)

GetResourceNode() returns null → falls through to IsValueProvider → generates StaticResourceExtension.ProvideValue(serviceProvider). The SimpleValueTargetProvider provides the full parent chain, and TryGetApplicationLevelResource checks Application.Current.Resources. The binding IS still compiled into a TypedBinding — only the converter resolution is deferred.

Verified on both main and net11.0

All tests pass on both branches.

Tests added

Test What it verifies
SourceGenResolvesConverterAtCompileTime_ImplicitResources Converter in implicit <Resources> → compile-time resolution, no ProvideValue
SourceGenResolvesConverterAtCompileTime_ExplicitResourceDictionary Converter in explicit <ResourceDictionary> → compile-time resolution, no ProvideValue
SourceGenCompilesBindingWithConverterToTypedBinding Converter NOT in page resources → still compiled to TypedBinding, no raw Binding fallback
BindingWithConverterFromAppResourcesWorksCorrectly × 3 Runtime behavior correct for all inflators (Runtime, XamlC, SourceGen)

Copilot AI review requested due to automatic review settings March 28, 2026 15:05
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 28, 2026

🚀 Dogfood this PR with:

⚠️ WARNING: Do not do this without first carefully reviewing the code of this PR to satisfy yourself it is safe.

curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 34717

Or

  • Run remotely in PowerShell:
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 34717"

Copy link
Copy Markdown
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

Adds a new XAML unit test for issue #34713 to validate that bindings within an x:DataType scope that include Converter={StaticResource ...} are still source-generated into TypedBinding and function correctly at runtime when the converter is provided via Application.Resources.

Changes:

  • Added issue test page Maui34713.xaml with an x:DataType-scoped binding using a StaticResource converter.
  • Added code-behind tests validating source-generator output includes TypedBinding (and does not fall back to Binding) and validating runtime behavior across inflators.

Reviewed changes

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

File Description
src/Controls/tests/Xaml.UnitTests/Issues/Maui34713.xaml Repro XAML: x:DataType scope with a StaticResource converter binding.
src/Controls/tests/Xaml.UnitTests/Issues/Maui34713.xaml.cs Adds tests that inspect generated source and validate converter resolution via Application.Resources for Runtime/XamlC/SourceGen.

Comment on lines +3 to +6
using Microsoft.Maui.ApplicationModel;
using Microsoft.Maui.Controls.Core.UnitTests;
using Microsoft.Maui.Controls.Internals;
using Microsoft.Maui.Dispatching;
Copy link

Copilot AI Mar 28, 2026

Choose a reason for hiding this comment

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

using Microsoft.Maui.Controls.Internals; appears to be unused in this file (no symbols from that namespace are referenced). With analyzers enabled, unused usings can fail the build; please remove it (or reference the intended type explicitly if it was meant to be used).

Copilot uses AI. Check for mistakes.
@github-actions
Copy link
Copy Markdown
Contributor

🧪 PR Test Evaluation

Overall Verdict: ⚠️ Tests need improvement

This PR adds solid XAML unit tests as regression coverage for issue #34713 (Binding with Converter and x:DataType). The tests correctly use xUnit with [XamlInflatorData], and the happy path is well covered. The main gaps are a missing error-path test (converter not found) and some brittleness in the SourceGen string assertions.

👍 / 👎 — Was this evaluation helpful? React to let us know!

📊 Expand Full Evaluation

PR Test Evaluation Report

PR: #34717 — Add regression test for #34713: Binding with Converter and x:DataType
Test files evaluated: 2 (Maui34713.xaml, Maui34713.xaml.cs)
Fix files: 0 (test-only PR — regression tests for confirmed-correct behavior)


Overall Verdict

⚠️ Tests need improvement

The happy path is well covered and the SourceGen output check is valuable, but there is a missing error-path edge case and the SourceGen string assertions are somewhat brittle.


1. Fix Coverage — ⚠️ Concern

This is a test-only PR adding regression tests for confirmed-correct behavior. The commit message states: "Investigation shows the current source generator correctly compiles this scenario." There are no production code changes to cover.

The tests do verify the exact behavior at the center of issue #34713: that a Binding using Converter={StaticResource ...} alongside x:DataType is compiled into a TypedBinding by the source generator (not a runtime Binding). As regression tests, they serve their purpose.


2. Edge Cases & Gaps — ⚠️ Concern

Covered:

  • SourceGen generates TypedBinding (not raw Binding) when converter is referenced via StaticResource + x:DataType ✅
  • Converter resolved from Application.Resources at runtime works correctly for all inflators (Runtime, XamlC, SourceGen) ✅
  • Binding propagates correctly once BindingContext is set ✅

Missing:

  • Converter not found in resources: What happens at runtime when BoolToTextConverter is not registered in Application.Resources? Issue Binding with Converter and x:DataType is not compiled — falls back to runtime and throws missing resource #34713 was about the converter not being found / the binding not working correctly. A test verifying the error behavior (or graceful fallback) would complete the coverage.
  • BindingContext already set on page creation: The test sets BindingContext after page construction. A test where BindingContext is set via XAML x:DataType at construction time (with resources pre-registered in the constructor) would validate a slightly different initialization order.

3. Test Type Appropriateness — ✅ Pass

Current: XAML Test (in Controls.Xaml.UnitTests)
Recommendation: XAML tests are exactly right here. The issue is about XAML source generation and runtime binding behavior — no heavier UI or device tests are needed.


4. Convention Compliance — ⚠️ Concern

  • File naming Maui34713 follows MauiXXXXX convention ✅ (note: the Gather-TestContext script reported a false positive here)
  • Uses xUnit ([Fact], [Theory], [Collection("Issue")], IDisposable) — consistent with the rest of the project ✅
  • Proper cleanup in Dispose (Application.SetCurrentApplication(null), DispatcherProvider.SetCurrent(null)) ✅
  • [XamlInflatorData] used correctly to run BindingWithConverterFromAppResourcesWorksCorrectly across all inflators ✅

Minor issue:

  • The #nullable enable directive appears in the middle of the file (after the partial class and Tests class definitions), just before the Maui34713ViewModel and Maui34713BoolToTextConverter helper classes at the bottom. While functionally harmless, the convention is to place #nullable enable at the top of the file.

5. Flakiness Risk — ✅ Low

No async/timing issues, no UI interactions, no screenshots, no Task.Delay or Thread.Sleep. Cleanup is properly done in Dispose. Risk is low.


6. Duplicate Coverage — ✅ No significant duplicates

Existing tests (e.g., Bz27863) test Converter={StaticResource ...} with x:DataType, but with the converter defined in page resources (XAML (ContentPage.Resources)). The new test specifically covers the Application-level resources scenario, which is the novel case from issue #34713. No duplication.


7. Platform Scope — ✅ Pass

XAML unit tests run cross-platform and test all three inflators (Runtime, XamlC, SourceGen). Appropriate for a XAML-level behavior fix.


8. Assertion Quality — ⚠️ Concern

BindingWithConverterFromAppResourcesWorksCorrectly: Assertions are specific and meaningful — checks actual label text values after binding propagation ("Active" / "Test"). ✅

SourceGenCompilesBindingWithConverterToTypedBinding: Assertions check specific substrings in generated code:

Assert.Contains("TypedBinding", generated, ...);
Assert.Contains("Converter = extension.Converter", generated, ...);
Assert.Contains("StaticResourceExtension", generated, ...);
Assert.Contains("ProvideValue", generated, ...);
Assert.DoesNotContain("new global::Microsoft.Maui.Controls.Binding(", generated, ...);

These are valuable for verifying correct codegen output. However, "Converter = extension.Converter" is a very specific code formatting string — if the source generator is refactored (e.g., variable renaming, whitespace changes), the assertion will break even if the behavior is still correct. Consider using a more resilient pattern, e.g. checking for the semantic shape rather than exact identifier names.


9. Fix-Test Alignment — ✅ Pass

Both tests target exactly the right behavior: TypedBinding generation when a Binding uses both Converter={StaticResource ...} and x:DataType. The [Fact] is SourceGen-only (correct — it inspects generated IL). The [Theory] + [XamlInflatorData] covers runtime behavior for all inflators (correct — validates end-to-end binding resolution).


Recommendations

  1. Add an error-path test: Add a test case that verifies behavior when the BoolToTextConverter is not registered in Application.Resources. Does it throw a helpful exception? Return a default value? This would complete the regression coverage for the bug scenario.

  2. Make SourceGen assertion more resilient: The check Assert.Contains("Converter = extension.Converter", ...) ties the test to the generated variable name extension. Consider asserting on the semantic outcome instead (e.g., just checking "Converter =" or verifying via the runtime test that the converter is actually invoked), so the test doesn't break on codegen refactors that don't change behavior.

  3. Move #nullable enable to line 1: Minor style issue — place #nullable enable at the top of the file, before the using statements, for consistency with the rest of the codebase.

Warning

⚠️ Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

  • dc.services.visualstudio.com

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "dc.services.visualstudio.com"

See Network Configuration for more information.

Note

🔒 Integrity filtering filtered 3 items

Integrity filtering activated and filtered the following items during workflow execution.
This happens when a tool call accesses a resource that does not meet the required integrity or secrecy level of the workflow.

🧪 Test evaluation by Evaluate PR Tests

@github-actions
Copy link
Copy Markdown
Contributor

🧪 PR Test Evaluation

Overall Verdict: ✅ Tests are adequate

This test-only PR adds focused XAML unit tests for issue #34713 (Binding with Converter + x:DataType in source generation). Tests use the lightest appropriate type, cover the core scenarios, and assert specifically on compiler output.

👍 / 👎 — Was this evaluation helpful? React to let us know!

📊 Expand Full Evaluation

PR Test Evaluation Report

PR: #34717 — Add regression tests for #34713: Binding with Converter and x:DataType
Test files evaluated: 2 (Maui34713.xaml, Maui34713.xaml.cs)
Fix files: 0 (test-only PR)


Overall Verdict

Tests are adequate

Four focused XAML unit tests cover the key source generator scenarios from issue #34713; assertions are specific and the chosen test type is optimal.


1. Fix Coverage — ✅

The tests target the exact scenarios from issue #34713:

  • Compile-time resolution when converter is in page resources (implicit and explicit ResourceDictionary)
  • Compile-time TypedBinding emission when converter is not in page resources (App.Resources)
  • Runtime correctness when converter lives in Application.Resources, tested across all inflators

All source gen assertions check that TypedBinding is emitted (not a plain Binding) and that ProvideValue is not called at compile time — directly reflecting the fix behavior. A regression in the source generator would cause these tests to fail.


2. Edge Cases & Gaps — ⚠️

Covered:

  • Converter in implicit ContentPage.Resources
  • Converter in explicit (ResourceDictionary) wrapper
  • Converter absent from page resources (supplied via Application.Resources at runtime)
  • Runtime binding evaluation for all three XamlInflator variants (Runtime, XamlC, SourceGen)

Missing (minor):

  • XamlC path for "converter in page resources" scenarios — the first two tests (ImplicitResources, ExplicitResourceDictionary) only exercise the SourceGen code path via RunMauiSourceGenerator. There are no corresponding MockCompiler.Compile checks to verify XamlC handles the same input correctly. Given the issue title specifically says "SourceGen", this gap is acceptable but worth noting.
  • Missing converter key — no negative test for the case where the StaticResource key is not found in page or app resources (expected exception or graceful fallback).

3. Test Type Appropriateness — ✅

Current: XAML Unit Tests (Controls.Xaml.UnitTests)
Recommendation: Same — this is the optimal choice. The issue is purely about compile-time code generation behavior; no device context or UI interaction is required.


4. Convention Compliance — ✅

  • ✅ File naming: Maui34713.xaml / Maui34713.xaml.cs — follows MauiXXXXX convention
  • [Collection("Issue")] on the test class — consistent with other issue tests in the project
  • [Fact] and [Theory] — correct for this xUnit-based test project
  • [XamlInflatorData] on the Theory test — correct attribute for iterating all inflators
  • IDisposable teardown properly cleans up DispatcherProvider, AppInfo, and Application
  • ℹ️ The automated script flagged a false positive ("Maui34713.xaml doesn't follow MauiXXXXX pattern") — this is a script regex issue; the file name is correct

5. Flakiness Risk — ✅ Low

All tests are pure compile-time or in-memory execution — no async, no UI, no timing dependencies, no screenshots. Flakiness risk is negligible.


6. Duplicate Coverage — ✅ No duplicates

No existing tests in the Issues/ folder cover the combination of StaticResource converter resolution with x:DataType source generation. The closest prior test (Bz27863) covers converters in templates, not this scenario.


7. Platform Scope — ✅

XAML unit tests run as part of the cross-platform compilation pipeline and are not platform-specific. All three inflator modes (Runtime, XamlC, SourceGen) are covered by BindingWithConverterFromAppResourcesWorksCorrectly.


8. Assertion Quality — ✅

  • Source gen tests assert on the exact generated code: Assert.Contains("TypedBinding", ...), Assert.DoesNotContain(".ProvideValue(", ...), Assert.DoesNotContain("new global::Microsoft.Maui.Controls.Binding(", ...) — highly specific and will catch regressions
  • Runtime test asserts on actual text values ("Active", "Test") — verifies end-to-end binding evaluation

9. Fix-Test Alignment — ✅

Test scenarios directly mirror issue #34713: a Binding with Converter={StaticResource ...} inside an element with x:DataType, where the converter may or may not exist in page resources. This matches the fix's code paths.


Recommendations

  1. Optional – add XamlC coverage for "converter in page resources": Add a MockCompiler.Compile check alongside the two source gen tests to verify XamlC also handles these scenarios correctly. Not blocking.
  2. Optional – add a missing-key test: A test that verifies correct behavior (exception or runtime fallback) when the StaticResource key is absent from both page and app resources would improve confidence in error handling.

Warning

⚠️ Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

  • dc.services.visualstudio.com

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "dc.services.visualstudio.com"

See Network Configuration for more information.

Note

🔒 Integrity filtering filtered 3 items

Integrity filtering activated and filtered the following items during workflow execution.
This happens when a tool call accesses a resource that does not meet the required integrity or secrecy level of the workflow.

🧪 Test evaluation by Evaluate PR Tests

Adds XAML unit tests verifying the source generator correctly handles
bindings with Converter={StaticResource ...} inside x:DataType scopes.

Tests verify:
- When converter IS in page resources (implicit or explicit
  ResourceDictionary), the source generator resolves it at compile
  time — no runtime StaticResourceExtension.ProvideValue call.
- When converter is NOT in page resources, the binding is still compiled
  into a TypedBinding with runtime converter resolution.
- Converter works correctly at runtime for all inflators (Runtime,
  XamlC, SourceGen) when the resource is in Application.Resources.

Investigation shows the source generator correctly compiles this
scenario on both main and net11.0 branches.

Fixes #34713

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions
Copy link
Copy Markdown
Contributor

🧪 PR Test Evaluation

Overall Verdict: ⚠️ Tests need improvement

The tests cover the core bug scenario well — binding with Converter + x:DataType where the converter lives in Application.Resources — and correctly exercise all inflators. However, there are structural issues and meaningful edge case gaps worth addressing.

👍 / 👎 — Was this evaluation helpful? React to let us know!

📊 Expand Full Evaluation

PR Test Evaluation Report

PR: #34717 — Add regression tests for #34713: Binding with Converter and x:DataType
Test files evaluated: 2 (Maui34713.xaml, Maui34713.xaml.cs)
Fix files: 0 (test-only PR)


Overall Verdict

⚠️ Tests need improvement

The four tests cover the principal bug scenario and source-generator behavior well, but several edge cases are missing, two structural concerns warrant attention, and assertions checking raw generated-code strings are fragile.


1. Fix Coverage — ✅

The BindingWithConverterFromAppResourcesWorksCorrectly test directly reproduces the bug (converter in Application.Resources, not page resources, with x:DataType), exercises all three inflators (Runtime, XamlC, SourceGen), and asserts on the bound label text. SourceGenCompilesBindingWithConverterToTypedBinding verifies the source generator emits a TypedBinding rather than falling back to new Binding(…). Coverage of the actual broken code path is solid.


2. Edge Cases & Gaps — ⚠️

Covered:

  • Converter in implicit page resources (ContentPage.Resources, direct element)
  • Converter in explicit (ResourceDictionary) within page resources
  • Converter in Application.Resources (the primary bug scenario)
  • Binding without converter (label1 / Name binding)

Missing:

  • ConverterParameter{Binding IsActive, Converter={StaticResource BoolToTextConverter}, ConverterParameter=foo} is a common variant; the fix should handle it the same way, but it is not tested.
  • Converter not found at all — what is the expected behavior (silent fallback, diagnostic, exception) when the converter key is absent from both page and application resources? No test documents or guards this path.
  • Merged ResourceDictionary — converter defined in a merged dictionary ((ResourceDictionary.MergedDictionaries)) rather than inline is a common real-world pattern and a potential regression surface.

3. Test Type Appropriateness — ✅

Current: XAML Unit Tests (source-generator + runtime inflation)
Recommendation: Same — XAML tests are exactly right for compile-time source-generator and XAML-inflator behavior. No heavier test type is needed.


4. Convention Compliance — ⚠️

  • ✅ File naming Maui34713.xaml / Maui34713.xaml.cs follows MauiXXXXX convention. (The script's warning is a false positive.)
  • [Collection("Issue")] is the correct collection name for the Issues/ folder (matches Maui32924, Gh12763, Bz41296, etc.).
  • ✅ xUnit [Fact] / [Theory] + [XamlInflatorData] is the current framework convention for this project.
  • ✅ Constructor-for-setup + IDisposable.Dispose-for-teardown is the correct xUnit pattern.
  • ⚠️ internal test methods — all four test methods are marked internal. Existing tests in FindByName.xaml.cs use the same pattern, so it works with the project's xUnit configuration, but it is non-standard and worth flagging. Prefer public unless there is a deliberate reason.
  • ⚠️ #nullable enable mid-file — appears at line 188, enabling nullable context only for the two helper classes at the bottom. The rest of the file (including all test methods) runs without nullable annotations. Better to place it at the top of the file (line 1) to cover everything.

5. Flakiness Risk — ✅ Low

All tests are pure in-process compilation or XAML inflation — no Appium, no UI, no platform interactions. No Task.Delay or Thread.Sleep. Risk is negligible.


6. Duplicate Coverage — ✅ No duplicates

No existing test in Xaml.UnitTests exercises Binding with Converter + x:DataType where the converter is resolved from Application.Resources. The new tests fill a genuine gap.


7. Platform Scope — ✅

Source-generator and XAML compilation tests are inherently cross-platform. The [XamlInflatorData] attribute runs every inflator variant. No platform-specific concerns apply.


8. Assertion Quality — ⚠️

Test Assertion Quality
BindingWithConverterFromAppResourcesWorksCorrectly Assert.Equal("Active", page.label0.Text) ✅ Specific, behavioral
SourceGenCompilesBindingWithConverterToTypedBinding Assert.Contains("TypedBinding", generated) ⚠️ Checks generated-code string — brittle if codegen format changes
SourceGenCompilesBindingWithConverterToTypedBinding Assert.DoesNotContain("new global::Microsoft.Maui.Controls.Binding(", generated) ⚠️ Same fragility concern
SourceGenResolvesConverterAtCompileTime_* Assert.DoesNotContain(".ProvideValue(", generated) ⚠️ Same fragility — tests an optimization, not core correctness

The string-based generated-code assertions are the best available tool for source-generator tests (the MockSourceGenerator pattern is established in this codebase), so this is acceptable — but the tests may break if the source-generator template changes its output format even for unrelated reasons. Consider adding a comment noting this fragility.


9. Fix-Test Alignment — ✅

This is a test-only PR. The XAML file explicitly documents (in a comment) that the converter is not in page resources and is expected to come from Application.Resources at runtime — precisely the scenario described in issue #34713. The source-generator tests target the compile-time code paths in the XAML source generator responsible for TypedBinding emission and StaticResource resolution. Alignment is tight.


Recommendations

  1. Add ConverterParameter test — add a variant of BindingWithConverterFromAppResourcesWorksCorrectly (or a separate [Theory]) that includes ConverterParameter=… to guard against regressions in that code path.
  2. Move #nullable enable to line 1 — makes nullable annotations apply to the entire file, including all test methods and the partial class declaration.
  3. Make test methods public — change internal void to public void on all four test methods to align with xUnit best practices (even if internal currently works).
  4. (Optional) Add merged-dictionary scenario — a test where the converter is defined in a merged ResourceDictionary would cover a common real-world pattern and increase confidence in the fix's completeness.

Warning

⚠️ Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

  • dc.services.visualstudio.com

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "dc.services.visualstudio.com"

See Network Configuration for more information.

Note

🔒 Integrity filtering filtered 2 items

Integrity filtering activated and filtered the following items during workflow execution.
This happens when a tool call accesses a resource that does not meet the required integrity or secrecy level of the workflow.

🧪 Test evaluation by Evaluate PR Tests

@StephaneDelcroix StephaneDelcroix merged commit 53e6575 into main Mar 28, 2026
36 checks passed
@StephaneDelcroix StephaneDelcroix deleted the 34713-07fa branch March 28, 2026 19:23
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.

Binding with Converter and x:DataType is not compiled — falls back to runtime and throws missing resource

2 participants