test(Table): add unload BootstrapBlazor_DynamicAssembly unit test#7909
Merged
test(Table): add unload BootstrapBlazor_DynamicAssembly unit test#7909
Conversation
Contributor
Reviewer's GuideAdds new unit tests around Table dynamic context selection behavior and BootstrapBlazor_DynamicAssembly unloadability, and slightly refactors checkbox markup to support dynamic-object GUID-based selection correctly. Sequence diagram for Table row selection with dynamic objectssequenceDiagram
participant User as actor_User
participant Table as TableComponent
participant Item as RowItem
participant Checkbox as CheckboxComponent
User->>Table: Trigger row selection (click header/row)
Table->>Item: Evaluate type
alt Item implements IDynamicObject
Table->>Checkbox: Render with Value = item.DynamicObjectPrimaryKey
Table->>Checkbox: Set State = RowCheckState(item)
Table->>Checkbox: Set OnStateChanged = OnCheckGuid
else Regular item
Table->>Checkbox: Render with Value = item
Table->>Checkbox: Set State = RowCheckState(item)
Table->>Checkbox: Set OnStateChanged = OnCheck
end
User->>Checkbox: Click checkbox
Checkbox-->>Table: Invoke OnCheckGuid or OnCheck with new state
Table->>Table: Update selected row collection based on Value
Table-->>User: UI reflects updated selection state
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
bb-auto
Bot
added
bug
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The
DynamicContext_ChangeDetection_Oktest relies on the internal type name and private field_immutableObjectTypesCachevia reflection, which may be brittle across framework versions; consider centralizing these string literals (e.g., constants) and/or adding a defensive early-return if the type/field is not found rather than failing the test. - In
DynamicContext_ChangeDetection_Ok, the assembly name"BootstrapBlazor_DynamicAssembly"is hard-coded; consider using a shared constant or deriving it from the dynamic context/assembly creation logic to keep the test resilient to future renames.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `DynamicContext_ChangeDetection_Ok` test relies on the internal type name and private field `_immutableObjectTypesCache` via reflection, which may be brittle across framework versions; consider centralizing these string literals (e.g., constants) and/or adding a defensive early-return if the type/field is not found rather than failing the test.
- In `DynamicContext_ChangeDetection_Ok`, the assembly name `"BootstrapBlazor_DynamicAssembly"` is hard-coded; consider using a shared constant or deriving it from the dynamic context/assembly creation logic to keep the test resilient to future renames.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7909 +/- ##
===========================================
+ Coverage 99.98% 100.00% +0.01%
===========================================
Files 764 764
Lines 34337 34337
Branches 4711 4711
===========================================
+ Hits 34333 34337 +4
+ Misses 3 0 -3
+ Partials 1 0 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
Adds new unit tests around Table<DynamicObject> selection behavior (Guid-backed checkbox values) and a regression test intended to ensure dynamically emitted assemblies remain unloadable (Issue #7908).
Changes:
- Added bUnit tests for DynamicContext multi-select scenarios (CardView row toggle, header select-all, and
ShowRowCheckboxCallbackfiltering). - Added a regression test that inspects Blazor ChangeDetection’s internal immutable type cache to ensure
BootstrapBlazor_DynamicAssemblytypes are not retained. - Minor formatting-only change to CardView row checkbox markup in
Table.razor.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| test/UnitTest/Components/TableTest.cs | Adds new DynamicContext selection tests and a ChangeDetection cache regression test for dynamic assembly unloadability. |
| src/BootstrapBlazor/Components/Table/Table.razor | Reflows CardView checkbox markup (no functional change expected). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+6583
to
+6595
| var type = typeof(ComponentBase).Assembly.GetType("Microsoft.AspNetCore.Components.ChangeDetection"); | ||
| Assert.NotNull(type); | ||
|
|
||
| var fieldInfo = type.GetField("_immutableObjectTypesCache", System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Static); | ||
| Assert.NotNull(fieldInfo); | ||
|
|
||
| IEnumerable<Type>? items = null; | ||
| if (fieldInfo.GetValue(null) is ConcurrentDictionary<Type, bool> cache) | ||
| { | ||
| items = cache.Keys.Where(i => i.Assembly.GetName().Name == "BootstrapBlazor_DynamicAssembly"); | ||
| } | ||
| Assert.NotNull(items); | ||
| Assert.Empty(items); |
There was a problem hiding this comment.
This test relies on Blazor internal implementation details (internal type name Microsoft.AspNetCore.Components.ChangeDetection and private static field _immutableObjectTypesCache, plus a concrete ConcurrentDictionary<Type, bool> shape). Because CI uses a floating SDK (10.0.x), a framework patch could rename/move the type/field or change the cache type and cause unrelated test failures. Consider rewriting the assertion to validate unload without depending on private runtime fields (e.g., keep a WeakReference to a dynamically emitted type/assembly, dispose the component, force GC, and assert it can be collected), or at least tolerate missing/changed members by conditionally skipping/failing with a clearer message.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Link issues
fixes #7908
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Add unit tests to verify table behavior with dynamic contexts and ensure dynamic assemblies are not retained by Blazor change detection caching.
Enhancements:
Tests: