Skip to content

test: implement integration test with RemoteConfig class#9

Merged
koukibadr merged 1 commit into
mainfrom
test/setup-integation-test-with-config
Jul 8, 2026
Merged

test: implement integration test with RemoteConfig class#9
koukibadr merged 1 commit into
mainfrom
test/setup-integation-test-with-config

Conversation

@koukibadr

@koukibadr koukibadr commented Jul 8, 2026

Copy link
Copy Markdown
Owner

Summary by CodeRabbit

  • New Features

    • Initialization now supports providing a prebuilt remote config for more flexible setup, especially in testing scenarios.
    • Theme generation now handles fallback themes more reliably when skin data is unavailable.
  • Bug Fixes

    • Added validation for API keys during startup to prevent invalid configuration from proceeding.
  • Tests

    • Updated and expanded automated coverage for initialization, remote-config handling, and fallback theme behavior.

@coderabbitai

coderabbitai Bot commented Jul 8, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@koukibadr, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 36 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 5a41ba33-a28b-423e-8820-b0c19f640370

📥 Commits

Reviewing files that changed from the base of the PR and between 333bd2f and e3b898d.

📒 Files selected for processing (5)
  • lib/flutter_skin.dart
  • lib/remote/fskin_remote_config.dart
  • pubspec.yaml
  • test/flutter_skin/flutter_skin_init_test.dart
  • test/flutter_skin/flutter_skin_integration_with_service.dart
📝 Walkthrough

Walkthrough

FlutterSkin.init and FskinRemoteConfig.init now accept optional injectable dependencies (remoteConfig and skinService, respectively) for testability. FskinRemoteConfig.init also validates apiKey via regex. Tests migrated from mockito to mocktail, and a new integration test file was added.

Changes

Testable Initialization Flow

Layer / File(s) Summary
FskinRemoteConfig validation and service injection
lib/remote/fskin_remote_config.dart
Adds apiKey regex validation, an optional injectable SkinService, a skinService field used in fetchConfig, and a resetInstance() test helper.
FlutterSkin.init dependency injection
lib/flutter_skin.dart
Adds an optional remoteConfig parameter to init, conditionally skips remote config initialization when provided, and removes the setRemoteConfig method in favor of resetInstance.
Mocktail migration and test updates
pubspec.yaml, test/flutter_skin/flutter_skin_init_test.dart
Replaces mockito with mocktail, updates setUp to pass remoteConfig, and updates stubbing style to thenAnswer.
New SkinService integration tests
test/flutter_skin/flutter_skin_integration_with_service.dart
Adds a new test file with MockSkinService and four async tests covering theme resolution, null-skin handling, and fallback theme behavior.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Possibly related PRs

  • koukibadr/flutter_skin#2: Both PRs change the FlutterSkin.init/FskinRemoteConfig.init initialization flow and remote-config fetching via SkinService.
  • koukibadr/flutter_skin#5: Both PRs modify lib/flutter_skin.dart and lib/remote/fskin_remote_config.dart around init/fetchConfig code paths.

Poem

A hop, a skip, a mock injected in,
No more mockito, mocktail wins!
ApiKey checked with a watchful eye,
resetInstance clears, tests fly by.
🐇 Themes bloom bright, fallback stands near—
This rabbit cheers, the code is clear!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title is relevant and accurately reflects the new RemoteConfig-focused integration test work, though it does not mention the supporting production code changes.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
lib/flutter_skin.dart (1)

115-118: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

resetInstance() leaks WidgetsBindingObserver — add removeObserver before nulling the instance.

FlutterSkin.init calls WidgetsBinding.instance.addObserver(_instance!) when creating a new instance, but resetInstance() only sets _instance = null without calling removeObserver. In the integration test suite, setUp calls resetInstance and each test calls init, so observers accumulate — 4 tests produce 4 registered observers referencing stale instances. If didChangeAppLifecycleState fires on an old observer, it operates on a detached remoteConfig and apiKey.

🔒 Proposed fix
 `@visibleForTesting`
 static void resetInstance() {
+  if (_instance != null) {
+    WidgetsBinding.instance.removeObserver(_instance!);
+  }
   _instance = null;
 }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@lib/flutter_skin.dart` around lines 115 - 118, `FlutterSkin.resetInstance()`
is clearing `_instance` without unregistering the existing
`WidgetsBindingObserver`, so update it to remove the current instance from
`WidgetsBinding.instance` before setting the singleton to null. Use the
`FlutterSkin.init`/`_instance` pairing to locate the lifecycle registration and
ensure `removeObserver` is called only when an instance exists, preventing stale
observers from accumulating across tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@lib/remote/fskin_remote_config.dart`:
- Around line 49-55: The null check after `_instance ??= FskinRemoteConfig._()`
in `FskinRemoteConfig.init()` is unreachable because the assignment always
initializes the singleton. Remove the dead `if (_instance == null)`/throw block
and keep the method focused on ensuring `_instance` is created via
`FskinRemoteConfig._()`, so the initialization contract is clear and consistent.

In `@test/flutter_skin/flutter_skin_init_test.dart`:
- Around line 22-28: The test setup in FlutterSkin.init is reusing singleton
state, which can leak observers and instance data across cases. Update the setUp
block to call FlutterSkin.resetInstance() and FskinRemoteConfig.resetInstance()
before invoking FlutterSkin.init, matching the isolation pattern used in the
integration tests. Keep the reset calls in the test lifecycle setup so each test
starts from a clean FlutterSkin._instance state.

In `@test/flutter_skin/flutter_skin_integration_with_service.dart`:
- Line 18: The shared group-level MockSkinService instance in
flutter_skin_integration_with_service.dart can leak stubs between tests because
mocktail does not clear them automatically. Move MockSkinService creation into
each individual test (or set it up in per-test setup) so every test gets a fresh
instance, and keep the fetchData stubbing local to the test that uses it. Use
the MockSkinService variable near the integration test setup to locate and
update the shared fixture.

---

Outside diff comments:
In `@lib/flutter_skin.dart`:
- Around line 115-118: `FlutterSkin.resetInstance()` is clearing `_instance`
without unregistering the existing `WidgetsBindingObserver`, so update it to
remove the current instance from `WidgetsBinding.instance` before setting the
singleton to null. Use the `FlutterSkin.init`/`_instance` pairing to locate the
lifecycle registration and ensure `removeObserver` is called only when an
instance exists, preventing stale observers from accumulating across tests.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 0f111aa0-d956-4885-8301-83eb6294c8d0

📥 Commits

Reviewing files that changed from the base of the PR and between c399fac and 333bd2f.

📒 Files selected for processing (5)
  • lib/flutter_skin.dart
  • lib/remote/fskin_remote_config.dart
  • pubspec.yaml
  • test/flutter_skin/flutter_skin_init_test.dart
  • test/flutter_skin/flutter_skin_integration_with_service.dart

Comment thread lib/remote/fskin_remote_config.dart Outdated
Comment thread test/flutter_skin/flutter_skin_init_test.dart
Comment thread test/flutter_skin/flutter_skin_integration_with_service.dart Outdated
@koukibadr koukibadr force-pushed the test/setup-integation-test-with-config branch from 333bd2f to e3b898d Compare July 8, 2026 10:26
@koukibadr koukibadr merged commit ca104db into main Jul 8, 2026
4 checks passed
@koukibadr koukibadr deleted the test/setup-integation-test-with-config branch July 8, 2026 10:32
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.

1 participant