Skip to content

Commit

Permalink
fix: SentryWidgetsFlutterBinding initializing even if a binding alrea…
Browse files Browse the repository at this point in the history
…dy exists (#2494)

* add additional test case

* larger runner

* match ci like in main branch
  • Loading branch information
buenaflor authored Dec 13, 2024
1 parent 617ae18 commit 7b4ce83
Show file tree
Hide file tree
Showing 8 changed files with 95 additions and 48 deletions.
4 changes: 2 additions & 2 deletions .github/workflows/flutter.yml
Original file line number Diff line number Diff line change
Expand Up @@ -84,13 +84,13 @@ jobs:
if: matrix.target == 'web'
run: |
flutter test --platform chrome --test-randomize-ordering-seed=random --exclude-tags canvasKit
flutter test --platform chrome --test-randomize-ordering-seed=random --tags canvasKit --web-renderer canvaskit
flutter test --platform chrome --test-randomize-ordering-seed=random --tags canvasKit
- name: Test web (WASM)
if: matrix.target == 'web'
run: |
flutter test --platform chrome --wasm --test-randomize-ordering-seed=random --exclude-tags canvasKit
flutter test --platform chrome --wasm --test-randomize-ordering-seed=random --tags canvasKit --web-renderer canvaskit
flutter test --platform chrome --wasm --test-randomize-ordering-seed=random --tags canvasKit
- name: Test VM with coverage
if: matrix.target == 'linux' || matrix.target == 'macos' || matrix.target == 'windows'
Expand Down
56 changes: 28 additions & 28 deletions .github/workflows/flutter_test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ jobs:
access_token: ${{ github.token }}

test-android:
runs-on: macos-13
runs-on: ubuntu-latest
timeout-minutes: 30
defaults:
run:
Expand All @@ -38,6 +38,12 @@ jobs:
- name: checkout
uses: actions/checkout@v4

- name: Enable KVM group perms
run: |
echo 'KERNEL=="kvm", GROUP="kvm", MODE="0666", OPTIONS+="static_node=kvm"' | sudo tee /etc/udev/rules.d/99-kvm4all.rules
sudo udevadm control --reload-rules
sudo udevadm trigger --name-match=kvm
- uses: actions/setup-java@v4
with:
distribution: "adopt"
Expand All @@ -56,28 +62,7 @@ jobs:
- name: Gradle cache
uses: gradle/gradle-build-action@ac2d340dc04d9e1113182899e983b5400c17cda1 # pin@v3.0.0

- name: AVD cache
uses: actions/cache@v4
id: avd-cache
with:
path: |
~/.android/avd/*
~/.android/adb*
key: avd-31

- name: create AVD and generate snapshot for caching
if: steps.avd-cache.outputs.cache-hit != 'true'
uses: reactivecircus/android-emulator-runner@62dbb605bba737720e10b196cb4220d374026a6d #pin@v2.33.0
with:
working-directory: ./flutter/example
api-level: 31
profile: Nexus 6
arch: x86_64
force-avd-creation: false
avd-name: macOS-avd-x86_64-31
emulator-options: -no-snapshot-save -no-window -gpu swiftshader_indirect -noaudio -no-boot-anim -camera-back none
disable-animations: true
script: echo 'Generated AVD snapshot for caching.'
# TODO: fix emulator caching, in ubuntu-latest emulator won't boot: https://github.com/ReactiveCircus/android-emulator-runner/issues/278

- name: build apk
working-directory: ./flutter/example/android
Expand All @@ -91,8 +76,8 @@ jobs:
profile: Nexus 6
arch: x86_64
force-avd-creation: false
avd-name: macOS-avd-x86_64-31
emulator-options: -no-snapshot-save -no-window -gpu swiftshader_indirect -noaudio -no-boot-anim -camera-back none
avd-name: avd-x86_64-31
emulator-options: -no-snapshot-save -no-window -accel on -gpu swiftshader_indirect -noaudio -no-boot-anim -camera-back none
disable-animations: true
script: ./gradlew testDebugUnitTest

Expand All @@ -104,11 +89,24 @@ jobs:
profile: Nexus 6
arch: x86_64
force-avd-creation: false
avd-name: macOS-avd-x86_64-31
emulator-options: -no-snapshot-save -no-window -gpu swiftshader_indirect -noaudio -no-boot-anim -camera-back none
avd-name: avd-x86_64-31
emulator-options: -no-snapshot-save -no-window -accel on -gpu swiftshader_indirect -noaudio -no-boot-anim -camera-back none
disable-animations: true
script: flutter test integration_test/all.dart --verbose

- name: launch android emulator & run android integration test in profile mode
uses: reactivecircus/android-emulator-runner@62dbb605bba737720e10b196cb4220d374026a6d #pin@v2.33.0
with:
working-directory: ./flutter/example
api-level: 31
profile: Nexus 6
arch: x86_64
force-avd-creation: false
avd-name: avd-x86_64-31
emulator-options: -no-snapshot-save -no-window -accel on -gpu swiftshader_indirect -noaudio -no-boot-anim -camera-back none
disable-animations: true
script: flutter drive --driver=integration_test/test_driver/driver.dart --target=integration_test/sentry_widgets_flutter_binding_test.dart --profile -d emulator-5554

cocoa:
name: "${{ matrix.target }} | ${{ matrix.sdk }}"
runs-on: macos-latest-xlarge
Expand Down Expand Up @@ -158,7 +156,9 @@ jobs:
- name: run integration test
# Disable flutter integration tests for iOS for now (https://github.com/getsentry/sentry-dart/issues/1605#issuecomment-1695809346)
if: ${{ matrix.target != 'ios' }}
run: flutter test -d "${{ steps.device.outputs.name }}" integration_test/all.dart --verbose
run: |
flutter test -d "${{ steps.device.outputs.name }}" integration_test/all.dart --verbose
flutter drive --driver=integration_test/test_driver/driver.dart --target=integration_test/sentry_widgets_flutter_binding_test.dart --profile -d "${{ steps.device.outputs.name }}"
- name: run native test
# We only have the native unit test package in the iOS xcodeproj at the moment.
Expand Down
7 changes: 7 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,12 @@
# Changelog

## Unreleased


### Fixes

- SentryWidgetsFlutterBinding initializing even if a binding already exists ([#2494](https://github.com/getsentry/sentry-dart/pull/2494))

## 8.11.0

### Features
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
// ignore_for_file: avoid_print
// ignore_for_file: invalid_use_of_internal_member

import 'package:flutter_test/flutter_test.dart';
import 'package:integration_test/integration_test.dart';
import 'package:sentry_flutter/sentry_flutter.dart';

// This needs needs to be run with:
//
// flutter drive \
// --driver=integration_test/test_driver/driver.dart \
// --target=integration_test/sentry_widgets_flutter_binding_test.dart --profile
//
// This test case will NOT fail in debug builds, hence why we need to test it
// in at least a profile build.

void main() {
final originalBinding =
IntegrationTestWidgetsFlutterBinding.ensureInitialized();

group('$SentryWidgetsFlutterBinding', () {
testWidgets('return existing binding', (tester) async {
final binding = SentryWidgetsFlutterBinding.ensureInitialized();

expect(binding, equals(originalBinding));
});
});
}
3 changes: 3 additions & 0 deletions flutter/example/integration_test/test_driver/driver.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
import 'package:integration_test/integration_test_driver.dart';

Future<void> main() => integrationDriver();
25 changes: 7 additions & 18 deletions flutter/lib/src/binding_wrapper.dart
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
// ignore_for_file: invalid_use_of_internal_member

import 'package:flutter/foundation.dart';
import 'package:flutter/widgets.dart';
import 'package:meta/meta.dart';

Expand Down Expand Up @@ -48,32 +47,22 @@ WidgetsBinding? _ambiguate(WidgetsBinding? binding) => binding;

class SentryWidgetsFlutterBinding extends WidgetsFlutterBinding
with SentryWidgetsBindingMixin {
@override
void initInstances() {
super.initInstances();
_instance = this;
}

static SentryWidgetsFlutterBinding get instance =>
BindingBase.checkInstance(_instance);
static SentryWidgetsFlutterBinding? _instance;

/// Returns an instance of [SentryWidgetsFlutterBinding].
/// If no binding has yet been initialized, creates and initializes one.
///
/// If the binding was already initialized with a different implementation,
/// returns the existing [WidgetsBinding] instance instead.
static WidgetsBinding ensureInitialized() {
try {
if (SentryWidgetsFlutterBinding._instance == null) {
SentryWidgetsFlutterBinding();
}
return SentryWidgetsFlutterBinding.instance;
} catch (e) {
// Try to get the existing binding instance
return WidgetsBinding.instance;
} catch (_) {
Sentry.currentHub.options.logger(
SentryLevel.info,
'WidgetsFlutterBinding already initialized. '
'Falling back to default WidgetsBinding instance.');
'WidgetsFlutterBinding has not been initialized yet. '
'Creating $SentryWidgetsFlutterBinding.');
// No binding exists yet, create our custom one
SentryWidgetsFlutterBinding();
return WidgetsBinding.instance;
}
}
Expand Down
20 changes: 20 additions & 0 deletions flutter/test/sentry_widgets_flutter_binding_test.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
import 'package:flutter/widgets.dart';
import 'package:flutter_test/flutter_test.dart';
import 'package:sentry_flutter/src/binding_wrapper.dart';

// Note: testing that SentryWidgetsFlutterBinding.ensureInitialized() returns
// the existing binding if one exists is not reliable to test in debug builds
// A previous faulty implementation was passing in debug but not in profile/release builds
// See flutter/example/integration_test/sentry_widgets_flutter_binding_test.dart

void main() {
group('$SentryWidgetsFlutterBinding', () {
test(
'no existing binding: ensureInitialized() returns SentryWidgetsFlutterBinding binding',
() {
final binding = SentryWidgetsFlutterBinding.ensureInitialized();

expect(binding, equals(WidgetsBinding.instance));
});
});
}

0 comments on commit 7b4ce83

Please sign in to comment.