Add regression test for #34713: Binding with Converter and x:DataType is compiled#34717
Add regression test for #34713: Binding with Converter and x:DataType is compiled#34717StephaneDelcroix merged 1 commit intomainfrom
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 34717Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 34717" |
There was a problem hiding this comment.
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.xamlwith anx:DataType-scoped binding using aStaticResourceconverter. - Added code-behind tests validating source-generator output includes
TypedBinding(and does not fall back toBinding) 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. |
| using Microsoft.Maui.ApplicationModel; | ||
| using Microsoft.Maui.Controls.Core.UnitTests; | ||
| using Microsoft.Maui.Controls.Internals; | ||
| using Microsoft.Maui.Dispatching; |
There was a problem hiding this comment.
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).
🧪 PR Test EvaluationOverall Verdict: 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
📊 Expand Full EvaluationPR Test Evaluation ReportPR: #34717 — Add regression test for #34713: Binding with Converter and x:DataType Overall VerdictThe 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 —
|
239a2df to
881353a
Compare
🧪 PR Test EvaluationOverall Verdict: ✅ Tests are adequate This test-only PR adds focused XAML unit tests for issue #34713 (Binding with Converter +
📊 Expand Full EvaluationPR Test Evaluation ReportPR: #34717 — Add regression tests for #34713: Binding with Converter and x:DataType 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:
All source gen assertions check that 2. Edge Cases & Gaps —
|
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>
881353a to
e945251
Compare
🧪 PR Test EvaluationOverall Verdict: The tests cover the core bug scenario well — binding with
📊 Expand Full EvaluationPR Test Evaluation ReportPR: #34717 — Add regression tests for #34713: Binding with Converter and x:DataType Overall VerdictThe 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 2. Edge Cases & Gaps —
|
| Test | Assertion | Quality |
|---|---|---|
BindingWithConverterFromAppResourcesWorksCorrectly |
Assert.Equal("Active", page.label0.Text) |
✅ Specific, behavioral |
SourceGenCompilesBindingWithConverterToTypedBinding |
Assert.Contains("TypedBinding", generated) |
|
SourceGenCompilesBindingWithConverterToTypedBinding |
Assert.DoesNotContain("new global::Microsoft.Maui.Controls.Binding(", generated) |
|
SourceGenResolvesConverterAtCompileTime_* |
Assert.DoesNotContain(".ProvideValue(", generated) |
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
- Add
ConverterParametertest — add a variant ofBindingWithConverterFromAppResourcesWorksCorrectly(or a separate[Theory]) that includesConverterParameter=…to guard against regressions in that code path. - Move
#nullable enableto line 1 — makes nullable annotations apply to the entire file, including all test methods and the partial class declaration. - Make test methods
public— changeinternal voidtopublic voidon all four test methods to align with xUnit best practices (even ifinternalcurrently works). - (Optional) Add merged-dictionary scenario — a test where the converter is defined in a merged
ResourceDictionarywould 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.
- pr:Add regression test for #34713: Binding with Converter and x:DataType is compiled #34717 (
pull_request_read: Resource 'pr:Add regression test for #34713: Binding with Converter and x:DataType is compiled #34717' has lower integrity than agent requires. Agent would need to drop integrity tags [unapproved:all approved:all] to trust this resource.) - issue:Binding with Converter and x:DataType is not compiled — falls back to runtime and throws missing resource #34713 (
issue_read: Resource 'issue:Binding with Converter and x:DataType is not compiled — falls back to runtime and throws missing resource #34713' has lower integrity than agent requires. Agent would need to drop integrity tags [approved:all unapproved:all] to trust this resource.)
🧪 Test evaluation by Evaluate PR Tests
Description
Adds regression tests for #34713 verifying the XAML source generator correctly handles bindings with
Converter={StaticResource ...}insidex:DataTypescopes.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, andProvideValueForStaticResourceExtensionreturns the variable directly — no runtimeProvideValuecall. The converter is referenced at compile time.When converter is NOT in page resources (runtime resolution ✅)
GetResourceNode()returns null → falls through toIsValueProvider→ generatesStaticResourceExtension.ProvideValue(serviceProvider). TheSimpleValueTargetProviderprovides the full parent chain, andTryGetApplicationLevelResourcechecksApplication.Current.Resources. The binding IS still compiled into aTypedBinding— only the converter resolution is deferred.Verified on both
mainandnet11.0All tests pass on both branches.
Tests added
SourceGenResolvesConverterAtCompileTime_ImplicitResources<Resources>→ compile-time resolution, noProvideValueSourceGenResolvesConverterAtCompileTime_ExplicitResourceDictionary<ResourceDictionary>→ compile-time resolution, noProvideValueSourceGenCompilesBindingWithConverterToTypedBindingTypedBinding, no rawBindingfallbackBindingWithConverterFromAppResourcesWorksCorrectly× 3