test(mudblazor): let every item-form builder configure its collection (#300) - #327
Merged
Merged
Conversation
…ection-exists-on-only-one-c
phmatray
marked this pull request as ready for review
August 13, 2026 13:44
…ection-exists-on-only-one-c
18 tasks
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.
Implements #300.
Closes #300.
Executing the implementation plan task-by-task; the checklist below — and the plan on the issue — are
ticked as each task lands. Opened as a draft — will be marked ready after the final task and a
code-review pass.
Plan
configureCollectionto the five single-field buildersconfigureCollectiontoRootFieldAndItemFormRenderOrderFormontoTextItemFormCI note — the one red test is pre-existing on
dev, not from this PRCollectionItemShapeGuardTests.No_Suite_Should_Re_Declare_A_Collection_Item_Shape_The_Fixture_Providesfails on this PR and on
devitself. Measured, not inferred:origin/dev@65944e5dev+3 tests (exactly the three self-tests added here), failures unchanged at 1. Running the guard class
against a pristine
origin/devcheckout reproduces it with byte-identical offenders:Two PRs that were each green in isolation collided once both landed on
dev: #306 added the guard,#308 added
FieldConfigurationRefreshTests.EqualityItemModel. Neither file is touched here.Code review
Ten findings; eight applied, two argued down.
Applied:
AllowReorder(), an independentsetter that reads the same whether the callback runs before or after the fixture's own
WithLabel/WithItemForm— so all six builders could have invoked it first and stayed green. Thespec calls that ordering the contract. Added
Each_Item_Forms_Collection_Callback_Should_Run_After_The_Fixtures_Own_Configuration, whichoverrides the label through
configureCollectionacross all seven builders (includingMultiFieldItemForm, whose ordering was never pinned either).Move uprenders withDisabled="@(index == 0)", so a one-row collection renders it permanently inert — "the buttonexists" was true of a form that cannot be reordered. Every model now carries two rows, and the
assertion checks row 0 disabled / row 1 enabled.
[Fact]s → one, matching the file's own precedent(
Each_Item_Form_Should_Apply_The_Callers_Configurationcovers five builders in one test). Net −6copies of the same comment and assertions; per-builder coverage and negative controls unchanged.
RenderOrderFormstill hand-rolled its render; now goes through the fixture'sRenderItemForm,the last such wiring in a suite that uses the helper everywhere else. Verified equivalent.
MultiFieldItemForm's remark still claimed to be the only builder reaching the collection; the sixnew parameters had no
<param>docs. All corrected, with the ordering contract written down.RootFieldAndItemForm's negative-control render.Argued down:
Collection<TModel,TItem>helper across the seven builders." This issueexplicitly rejected the combinator approach (Alternatives, approach C): the fixture's value is that
each builder is dumb and readable at the call site, and Adopt the shared collection-item fixture in the remaining test suites #258 showed unused indirection here costs
more than the duplication. Out of scope for a parameter addition.
RootFieldAndItemForm's label assertions duplicate an earlier test." They are the point ofTask 2 Step 1 — proving a third callback did not re-route the two already there. The earlier test
passes no
configureCollection, so it cannot catch that.Follow-ups
devshape-guard breakage —EqualityItemModelneeds either the fixture's model or anentry in
FindOffenders' allowlist with a reason. Deliberately not fixed here: it is outsidethis issue's test-only scope, and a
fix/dev-collection-item-shape-guardbranch already exists.next builder added here from omitting
configureCollection— which is exactly the regressionconfigureCollection exists on only one CollectionItemFixture builder #300 reports, one builder later. This repo's idiom for that is a reflection test (cf.
NativeRequiredBuilderTests): assert everyIFormConfiguration<>-returning member ofCollectionItemFixturedeclares a trailing optionalAction<CollectionFieldBuilder<,>>. Coversthe seven that exist and every future one; too much new surface to fold into this PR.