feat: expose from and origin for Override in tests #4368
Conversation
WalkthroughExposes a public Changes
Sequence Diagram(s)sequenceDiagram
participant Test
participant Override
participant ProviderBase
participant Family
Test->>Override: read .origin
activate Override
alt Provider override
Override->>ProviderBase: return origin (ProviderBase)
else Family override
Override->>Family: return origin (Family / from)
end
deactivate Override
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (4 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| extension on ProviderOverride { | ||
| /// The new provider behavior. | ||
| $ProviderBaseImpl<Object?> get providerOverride { | ||
| final that = this; |
There was a problem hiding this comment.
kept this private, as it's not required for solving #4302
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
packages/riverpod/lib/src/core/override.dart (1)
58-76: Add a merge helper to complement containsOverride.containsOverride is useful; consider a small helper to merge two override lists with “last wins” semantics to simplify test utilities.
Apply within ListOverrideX:
extension ListOverrideX on List<Override> { /// Returns `true` if the list already contains an [Override] for the family or provider that [override] overrides, `false` otherwise. bool containsOverride(Override override) { @@ } + + /// Returns a new list merging `this` with [additions], deduping by origin/from. + /// If duplicates exist, the last occurrence wins. + List<Override> mergedWith(Iterable<Override> additions) { + Object keyOf(Override o) => switch (o) { + ProviderOverride(:final origin) => (0, origin), + FamilyOverride(:final from) => (1, from), + _ => (2, o), + }; + final map = <Object, Override>{}; + for (final o in this) { + map[keyOf(o)] = o; + } + for (final o in additions) { + map[keyOf(o)] = o; + } + return List<Override>.unmodifiable(map.values); + } }packages/riverpod/CHANGELOG.md (1)
1-4: Add “visibleForTesting” note.Recommend noting these APIs are marked @VisibleForTesting to set expectations for production use.
website/docs/concepts2/overrides/provider_override.dart (1)
1-22: Good, minimal example.Imports and matcher usage are correct. Optionally add a one-liner showing ListOverrideX.containsOverride for discoverability.
website/docs/concepts2/overrides/default_overrides.dart (2)
37-43: Slightly cleaner merge without a manual loop.Equivalent and a bit more idiomatic; keeps
testOverridesmutable.- // ✅ Add default overrides only if not already present in test overrides - for (final defaultOverride in defaultOverrides) { - if (!testOverrides.containsOverride(defaultOverride)) { - testOverrides.add(defaultOverride); - } - } + // ✅ Add defaults only when missing + testOverrides.addAll( + defaultOverrides.where((o) => !testOverrides.containsOverride(o)), + );
44-46: Strengthen the assertion to demonstrate precedence, not just count.Optional: assert which overrides won. This makes the example self-validating.
// settingsProvider was added from defaults, but auth and user use test-specific values - expect(testOverrides.length, 3); + expect(testOverrides.length, 3); + // Optional: prove effective precedence + final container = ProviderContainer(overrides: testOverrides); + addTearDown(container.dispose); + expect(container.read(authProvider), customAuth); + expect(container.read(userFamily('42')), customUser); + expect(container.read(settingsProvider), defaultSettings);packages/riverpod/lib/src/core/provider_container.dart (4)
46-48: Remove stale lint ignore on public type.
ProviderOverrideis now public; the// ignore: library_private_types_in_public_apiis no longer needed.- // ignore: library_private_types_in_public_api - ProviderOverride? providerOverride; + ProviderOverride? providerOverride;
140-142: Same here: drop the unnecessary ignore.
FamilyOverrideis public now; keep the field clean.- // ignore: library_private_types_in_public_api - FamilyOverride? familyOverride; + FamilyOverride? familyOverride;
147-149: And here: parameter no longer needs the ignore.The type is public.
- void addProviderOverride( - // ignore: library_private_types_in_public_api - ProviderOverride override, { + void addProviderOverride( + ProviderOverride override, {
759-776: Developer UX: hint how to resolve duplicates in the debug error.Consider pointing devs to
ListOverrideX.containsOverrideso they can pre-merge overrides (the approach documented in this PR).- throw AssertionError( - 'Tried to override a provider twice within the same container: ${override.origin}', - ); + throw AssertionError( + 'Tried to override a provider twice within the same container: ${override.origin}. ' + 'If composing override lists for tests, merge them using ' + 'List<Override>.containsOverride(...) to avoid duplicates.', + );And similarly for the family error message below.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
packages/flutter_riverpod/CHANGELOG.md(1 hunks)packages/flutter_riverpod/lib/misc.dart(1 hunks)packages/hooks_riverpod/CHANGELOG.md(1 hunks)packages/hooks_riverpod/lib/misc.dart(1 hunks)packages/riverpod/CHANGELOG.md(1 hunks)packages/riverpod/lib/misc.dart(1 hunks)packages/riverpod/lib/src/core/family.dart(1 hunks)packages/riverpod/lib/src/core/override.dart(6 hunks)packages/riverpod/lib/src/core/provider/provider.dart(1 hunks)packages/riverpod/lib/src/core/provider_container.dart(9 hunks)packages/riverpod/test/src/core/overrides_test.dart(3 hunks)website/docs/concepts2/overrides.mdx(3 hunks)website/docs/concepts2/overrides/default_overrides.dart(1 hunks)website/docs/concepts2/overrides/duplicate_override_error.dart(1 hunks)website/docs/concepts2/overrides/family_override.dart(1 hunks)website/docs/concepts2/overrides/provider_override.dart(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build (master, packages/hooks_riverpod)
- GitHub Check: build (master, examples/todos)
- GitHub Check: build (stable, examples/marvel)
- GitHub Check: check_generation
🔇 Additional comments (24)
packages/riverpod/lib/src/core/override.dart (5)
40-44: FamilyOverride + from() exposure aligns with Family implementing it.Pattern-matching on Family() and $FamilyOverride() is correct; enables testing utilities.
Also applies to: 45-56
88-108: $ProviderOverride implements ProviderOverride — toString() UX is nice.Human-friendly overrideWithValue(...)/overrideWith(...) formatting is helpful in logs.
129-147: $FamilyOverride public factory with internal impl is appropriate.Good separation via factory to _FamilyOverrideImpl; tags keep it internal for non-testing use.
11-15: ProviderBase correctly implements ProviderOverride; extension pattern matching is sound.Verification confirms ProviderBase (packages/riverpod/lib/src/core/provider/provider.dart:25-29) implements ProviderOverride. The sealed class polymorphism enables the extension methods (override.dart:18-36) to cleanly pattern-match direct providers ($ProviderBaseImpl) and wrapper overrides ($ProviderOverride), correctly exposing origin and providerOverride without duplication.
29-38: Keep providerOverride getter internal. Unnamed extension on ProviderOverride is library‑private, so although packages/riverpod/lib/misc.dart re-exports ProviderOverride, the providerOverride getter is not exposed externally. (dart.dev)packages/riverpod/lib/misc.dart (1)
15-19: Exports look right for testing.Only named extensions are re-exported; internal providerOverride getter stays hidden. LGTM.
packages/riverpod/lib/src/core/family.dart (1)
48-48: Family now implements FamilyOverride — verified safe, no private counterparts found.The search confirmed no lingering references to
_FamilyOverrideor_ProviderOverridein the codebase. The interface implementation is clean, and the change correctly enables the Family() branch in FamilyOverrideX and ListOverrideX without conflicts. No functional risk.packages/flutter_riverpod/CHANGELOG.md (1)
1-3: LGTM! Clear changelog entry.The changelog entry accurately describes the new testing-facing APIs and properly credits the contributor.
website/docs/concepts2/overrides/family_override.dart (1)
10-19: LGTM! Clear demonstration of family override inspection.The test snippet effectively demonstrates how to inspect which family is being overridden using the
fromproperty.packages/riverpod/lib/src/core/provider/provider.dart (1)
29-29: LGTM! Key change exposing the override interface.This change makes the
ProviderOverrideinterface public, enabling the testing-focused functionality described in the PR. The change is minimal and non-breaking.website/docs/concepts2/overrides/duplicate_override_error.dart (1)
16-27: LGTM! Clear demonstration of the duplicate override error.The snippet effectively illustrates the problematic pattern that the new APIs help prevent. The ❌ marker clearly indicates this is an error scenario.
packages/riverpod/test/src/core/overrides_test.dart (4)
44-52: LGTM! Comprehensive test for origin property.The test properly validates that
overrideWithexposes the origin provider via theProviderOverrideinterface.
83-91: LGTM! Validates origin for overrideWithValue.The test ensures
overrideWithValuealso properly exposes the origin property, maintaining consistency across override methods.
126-135: LGTM! Validates family override inspection.The test correctly validates that family overrides expose the
fromproperty, enabling introspection of which family is being overridden.
138-222: LGTM! Thorough coverage of containsOverride behavior.The test suite comprehensively covers:
- Same provider detection
- Same family detection
- Empty list handling
- Mixed provider and family scenarios
This validates the core duplicate-detection functionality that addresses the PR objectives.
website/docs/concepts2/overrides.mdx (2)
11-14: LGTM! Proper import setup for new examples.The new imports correctly reference the example files demonstrating override inspection and merging patterns.
65-103: LGTM! Excellent documentation for the new testing APIs.The "Inspecting overrides" section clearly explains:
- The purpose of
originandfromproperties- How to use them for testing
- The
@visibleForTestingintent- Practical examples for provider and family overrides
- Solution to the duplicate override problem using
containsOverrideThis directly addresses the testing setup problem described in issue #4302.
packages/hooks_riverpod/CHANGELOG.md (1)
1-3: LGTM! Consistent changelog entry.The changelog entry matches the flutter_riverpod documentation and properly describes the exposed testing APIs.
packages/flutter_riverpod/lib/misc.dart (1)
15-19: LGTM! Proper public API exports.The new exports correctly expose the override inspection types and extension methods from
src/internals.dart, making them available for testing purposes as intended.packages/riverpod/lib/src/core/provider_container.dart (3)
334-360: Switch on Override is exhaustive—no issues found.Verification confirms that
Overrideinpackages/riverpod/lib/src/core/override.dartis markedsealed class Override {}with exactly two direct sealed subtypes:sealed class ProviderOverride implements Override {}andsealed class FamilyOverride implements Override {}. The switch statement in provider_container.dart correctly covers both variants, and Dart's sealed modifier guarantees exhaustiveness at compile time.
1062-1090: Verification confirms all override pattern-matching uses public types correctly.All six case statements matching override types in
provider_container.dart(lines 334, 336, 763, 769, 1062, 1079) consistently use the publicProviderOverride()andFamilyOverride()types. No internal override type patterns remain. The code update preserves semantics as intended.
198-209: Review comment verified—no changes needed.The code is correctly typed and stable:
- $FamilyOverride is an abstract class implementing FamilyOverride, and defines the abstract method
createElement($ProviderPointer pointer)- Both concrete implementations (TransitiveFamilyOverride and _FamilyOverrideImpl) properly implement createElement
- The only public FamilyOverride subtype is Family, which is handled separately by the third case pattern
(null, FamilyOverride() || null)usingorigin.$createElement(pointer)rather than the $FamilyOverride protocol- The pattern-match is type-safe and correctly dispatches between $FamilyOverride instances (with createElement) and other FamilyOverride subtypes
website/docs/concepts2/overrides/default_overrides.dart (1)
3-5: ListOverrideX and containsOverride are correctly exported—no issues found.Verification confirms:
ListOverrideXextension is defined inpackages/riverpod/lib/src/core/override.dart:60containsOverridemethod is defined in the extension at line 62- All three packages (
riverpod,hooks_riverpod,flutter_riverpod) exportListOverrideXfrommisc.dart- The doc file correctly imports
package:riverpod/misc.dartand usescontainsOverrideproperly- Code will compile correctly in both monorepo and published packages
packages/hooks_riverpod/lib/misc.dart (1)
15-19: Public exports verified and consistent.All five symbols are properly defined in
packages/riverpod/lib/src/core/override.dart(ProviderOverride, ProviderOverrideX, FamilyOverride, FamilyOverrideX, ListOverrideX) and exported identically from bothpackages/hooks_riverpod/lib/misc.dartandpackages/riverpod/lib/misc.dart, confirming no API skew.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/riverpod/lib/src/core/provider_container.dart (1)
366-400: Inconsistent type pattern in switch case.Line 384 still uses
$ProviderBaseImpl()in the switch pattern, while the rest of the PR systematically migrates toProviderBase<Object?>(). For consistency with the public type migration throughout this PR, consider updating this case.Apply this diff:
switch (dependency) { case Family(): final familyPointer = familyPointers[dependency]; if (familyPointer == null) return const []; return [familyPointer.targetContainer].followedBy( familyPointer.pointers.values.map((e) => e.targetContainer), ); - case $ProviderBaseImpl(): + case ProviderBase<Object?>(): return [ if (readPointer(dependency)?.targetContainer case final container?) container, ]; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/riverpod/lib/src/core/element.dart(1 hunks)packages/riverpod/lib/src/core/family.dart(1 hunks)packages/riverpod/lib/src/core/override.dart(5 hunks)packages/riverpod/lib/src/core/provider/provider.dart(1 hunks)packages/riverpod/lib/src/core/provider_container.dart(13 hunks)packages/riverpod/test/src/core/overrides_test.dart(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/riverpod/lib/src/core/family.dart
- packages/riverpod/test/src/core/overrides_test.dart
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: build (master, packages/riverpod_generator/integration/build_yaml)
- GitHub Check: build (master, packages/hooks_riverpod/example)
- GitHub Check: build (master, packages/riverpod)
- GitHub Check: build (master, packages/riverpod/example)
- GitHub Check: build (stable, packages/hooks_riverpod/example)
- GitHub Check: build (stable, packages/riverpod)
- GitHub Check: build (stable, packages/flutter_riverpod)
- GitHub Check: build (stable, examples/counter)
- GitHub Check: check_generation
- GitHub Check: riverpod_lint (master, packages/riverpod_lint_flutter_test)
- GitHub Check: riverpod_lint (stable, packages/riverpod_analyzer_utils_tests)
- GitHub Check: riverpod_lint (stable, packages/riverpod_analyzer_utils)
- GitHub Check: riverpod_lint (stable, packages/riverpod_lint_flutter_test)
🔇 Additional comments (9)
packages/riverpod/lib/src/core/provider/provider.dart (1)
50-51: LGTM! Clean implementation of the origin property.The getter correctly returns
thisfor the base provider case, providing a consistent interface for override origin tracking.packages/riverpod/lib/src/core/element.dart (1)
540-540: LGTM! Signature updated to use public base type.The parameter type change from
$ProviderBaseImpl<StateT>toProviderBase<StateT>aligns with the broader migration to public-facing types throughout the PR.packages/riverpod/lib/src/core/override.dart (3)
9-13: LGTM! Clean public API for override origin.The
originproperty is appropriately marked as@visibleForTesting, which aligns with the PR objective of enabling test utilities to inspect and merge overrides based on their origin.
15-36: LGTM! Consistent migration to public types.The migration from
$ProviderBaseImpltoProviderBasein both theorigingetter and theproviderOverrideextension is consistent and correct. The switch patterns properly handle bothProviderBase()and$ProviderOverride()cases.
129-130: LGTM! Family override origin exposure is consistent.Both
TransitiveFamilyOverrideand_FamilyOverrideImplnow exposeoriginby returningfrom, maintaining consistency with the provider override pattern.Also applies to: 153-154
packages/riverpod/lib/src/core/provider_container.dart (4)
39-39: LGTM! Origin field migrated to public type.The change from
$ProviderBaseImpl<Object?>toProviderBase<Object?>aligns with the PR's goal of exposing public origin tracking for overrides.
142-142: LGTM! Pointers map updated to use public base type.The map key type change from
$ProviderBaseImpl<Object?>toProviderBase<Object?>is consistent with the broader migration to public-facing types.
161-180: LGTM! Systematic migration to public base types.All method signatures and parameter types have been consistently updated from
$ProviderBaseImpl<Object?>toProviderBase<Object?>, maintaining functional equivalence while exposing the public API surface.Also applies to: 190-210, 443-455, 461-475, 505-532
971-980: LGTM! Public API switch patterns and typedef updated.The switch patterns at lines 971 and 985, along with the
SetupOverridetypedef, have been correctly updated to useProviderBase<Object?>instead of$ProviderBaseImpl<Object?>.Also applies to: 984-991, 1307-1309
|
Hey @rrousselGit thanks a lot for the review! I gave exposing I'm unsure if this is the way to go. Happy to adjust based on your feedback. I'll add the CHANGELOG and better documentation on |
|
@rrousselGit friendly ping :) |
2 participants
Related Issues
fixes #4302
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]).I have updated the
CHANGELOG.mdof the relevant packages.Changelog files must be edited under the form:
If this contains new features or behavior changes,
I have updated the documentation to match those changes.
Summary by CodeRabbit
New Features
originproperty on providers, families, and overrides to expose the underlying provider/family being overridden.Tests
originis correctly exposed for various override scenarios.Chores