Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove renderer on Dart:io platforms #1723

Merged
merged 5 commits into from
Nov 14, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

### Fixes

- Flutter renderer information was removed on dart:io platforms since it didn't add the correct value ([#1723](https://github.com/getsentry/sentry-dart/pull/1723))
- Unsupported types with Expando ([#1690](https://github.com/getsentry/sentry-dart/pull/1690))

### Features
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,8 @@ class FlutterEnricherEventProcessor implements EventProcessor {
// ignore: deprecated_member_use
final hasRenderView = _widgetsBinding?.renderViewElement != null;

final renderer = _options.rendererWrapper.getRenderer()?.name;

return <String, String>{
'has_render_view': hasRenderView.toString(),
if (tempDebugBrightnessOverride != null)
Expand All @@ -149,7 +151,7 @@ class FlutterEnricherEventProcessor implements EventProcessor {
// Also always fails in tests.
// See https://github.com/flutter/flutter/issues/83919
// 'window_is_visible': _window.viewConfiguration.visible,
'renderer': _options.rendererWrapper.getRenderer().name,
if (renderer != null) 'renderer': renderer,
};
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,13 @@ class ScreenshotEventProcessor implements EventProcessor {
}

final renderer = _options.rendererWrapper.getRenderer();
if (renderer != FlutterRenderer.skia &&

if (_options.platformChecker.isWeb &&
renderer != FlutterRenderer.canvasKit) {
_options.logger(SentryLevel.debug,
'Cannot take screenshot with ${_options.rendererWrapper.getRenderer().name} renderer.');
_options.logger(
SentryLevel.debug,
'Cannot take screenshot with ${renderer?.name} renderer.',
);
return event;
}

Expand Down
2 changes: 1 addition & 1 deletion flutter/lib/src/renderer/html_renderer.dart
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import 'dart:js' as js;

import 'renderer.dart';

FlutterRenderer getRenderer() {
FlutterRenderer? getRenderer() {
return isCanvasKitRenderer ? FlutterRenderer.canvasKit : FlutterRenderer.html;
}

Expand Down
4 changes: 1 addition & 3 deletions flutter/lib/src/renderer/io_renderer.dart
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
import 'renderer.dart';

FlutterRenderer getRenderer() {
return FlutterRenderer.skia;
}
FlutterRenderer? getRenderer() => null;
10 changes: 9 additions & 1 deletion flutter/lib/src/renderer/renderer.dart
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,22 @@ import 'unknown_renderer.dart'

@internal
class RendererWrapper {
FlutterRenderer getRenderer() {
FlutterRenderer? getRenderer() {
return implementation.getRenderer();
}
}

enum FlutterRenderer {
/// https://skia.org/
skia,

/// https://docs.flutter.dev/perf/impeller
impeller,

/// https://docs.flutter.dev/platform-integration/web/renderers
canvasKit,

/// https://docs.flutter.dev/platform-integration/web/renderers
html,
unknown,
}
3 changes: 1 addition & 2 deletions flutter/lib/src/sentry_flutter.dart
Original file line number Diff line number Diff line change
Expand Up @@ -167,8 +167,7 @@ mixin SentryFlutter {
integrations.add(LoadImageListIntegration(channel));
}
final renderer = options.rendererWrapper.getRenderer();
if (renderer == FlutterRenderer.skia ||
renderer == FlutterRenderer.canvasKit) {
if (!platformChecker.isWeb || renderer == FlutterRenderer.canvasKit) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The HTML renderer is the only renderer where screenshots do not work. Therefore, it should be enough to check for !web and the CanvasKit renderer

Copy link
Contributor

@buenaflor buenaflor Nov 14, 2023

Choose a reason for hiding this comment

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

Do you know if we have it documented somewhere that HTML renderer isn't supported for screenshot?

Might be good to improve the docs for that as well some time since the docs on sentry.io don't specify that, but well, docs in general are still rough on sentry 😓

Copy link
Collaborator Author

@ueman ueman Nov 14, 2023

Choose a reason for hiding this comment

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

@denrase might know that, since he wrote it. Though, it's pretty well known that the backing API doesn't work on the HTML renderer

integrations.add(ScreenshotIntegration());
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,38 @@ void main() {
fixture = Fixture();
});

testWidgets('flutter context', (WidgetTester tester) async {
testWidgets('flutter context on dart:io', (WidgetTester tester) async {
if (kIsWeb) {
// widget tests don't support onPlatform config
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do you have any better idea than this workaround?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, don't think so but I'm fine with this as long as it's documented

// https://pub.dev/packages/test#platform-specific-configuration
return;
}
// These two values need to be changed inside the test,
// otherwise the Flutter test framework complains that these
// values are changed outside of a test.
debugBrightnessOverride = Brightness.dark;
debugDefaultTargetPlatformOverride = TargetPlatform.android;
final enricher = fixture.getSut(
binding: () => tester.binding,
);

final event = await enricher.apply(SentryEvent());

debugBrightnessOverride = null;
debugDefaultTargetPlatformOverride = null;

final flutterContext = event?.contexts['flutter_context'];
expect(flutterContext, isNotNull);
expect(flutterContext, isA<Map<String, String>>());
}, skip: !kIsWeb);

testWidgets('flutter context on web', (WidgetTester tester) async {
if (!kIsWeb) {
// widget tests don't support onPlatform config
// https://pub.dev/packages/test#platform-specific-configuration
return;
}

// These two values need to be changed inside the test,
// otherwise the Flutter test framework complains that these
// values are changed outside of a test.
Expand Down
41 changes: 22 additions & 19 deletions flutter/test/event_processor/screenshot_event_processor_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,15 @@ void main() {
});

Future<void> _addScreenshotAttachment(
WidgetTester tester, FlutterRenderer renderer, bool added,
{int? expectedMaxWidthOrHeight}) async {
WidgetTester tester,
FlutterRenderer? renderer, {
required bool isWeb,
required bool added,
int? expectedMaxWidthOrHeight,
}) async {
// Run with real async https://stackoverflow.com/a/54021863
await tester.runAsync(() async {
final sut = fixture.getSut(renderer);
final sut = fixture.getSut(renderer, isWeb);

await tester.pumpWidget(SentryScreenshotWidget(
child: Text('Catching Pokémon is a snap!',
Expand All @@ -48,55 +52,54 @@ void main() {
});
}

testWidgets('adds screenshot attachment with skia renderer', (tester) async {
await _addScreenshotAttachment(tester, FlutterRenderer.skia, true);
testWidgets('adds screenshot attachment dart:io', (tester) async {
await _addScreenshotAttachment(tester, null, added: true, isWeb: false);
});

testWidgets('adds screenshot attachment with canvasKit renderer',
(tester) async {
await _addScreenshotAttachment(tester, FlutterRenderer.canvasKit, true);
await _addScreenshotAttachment(tester, FlutterRenderer.canvasKit,
added: true, isWeb: true);
});

testWidgets('does not add screenshot attachment with html renderer',
(tester) async {
await _addScreenshotAttachment(tester, FlutterRenderer.html, false);
});

testWidgets('does not add screenshot attachment with unknown renderer',
(tester) async {
await _addScreenshotAttachment(tester, FlutterRenderer.unknown, false);
await _addScreenshotAttachment(tester, FlutterRenderer.html,
added: false, isWeb: true);
});

testWidgets('does add screenshot in correct resolution for low',
(tester) async {
final height = SentryScreenshotQuality.low.targetResolution()!;
fixture.options.screenshotQuality = SentryScreenshotQuality.low;
await _addScreenshotAttachment(tester, FlutterRenderer.skia, true,
expectedMaxWidthOrHeight: height);
await _addScreenshotAttachment(tester, null,
added: true, isWeb: false, expectedMaxWidthOrHeight: height);
});

testWidgets('does add screenshot in correct resolution for medium',
(tester) async {
final height = SentryScreenshotQuality.medium.targetResolution()!;
fixture.options.screenshotQuality = SentryScreenshotQuality.medium;
await _addScreenshotAttachment(tester, FlutterRenderer.skia, true,
expectedMaxWidthOrHeight: height);
await _addScreenshotAttachment(tester, null,
added: true, isWeb: false, expectedMaxWidthOrHeight: height);
});

testWidgets('does add screenshot in correct resolution for high',
(tester) async {
final widthOrHeight = SentryScreenshotQuality.high.targetResolution()!;
fixture.options.screenshotQuality = SentryScreenshotQuality.high;
await _addScreenshotAttachment(tester, FlutterRenderer.skia, true,
expectedMaxWidthOrHeight: widthOrHeight);
await _addScreenshotAttachment(tester, null,
added: true, isWeb: false, expectedMaxWidthOrHeight: widthOrHeight);
});
}

class Fixture {
SentryFlutterOptions options = SentryFlutterOptions(dsn: fakeDsn);

ScreenshotEventProcessor getSut(FlutterRenderer flutterRenderer) {
ScreenshotEventProcessor getSut(
FlutterRenderer? flutterRenderer, bool isWeb) {
options.rendererWrapper = MockRendererWrapper(flutterRenderer);
options.platformChecker = MockPlatformChecker(isWebValue: isWeb);
return ScreenshotEventProcessor(options);
}
}
4 changes: 2 additions & 2 deletions flutter/test/mocks.dart
Original file line number Diff line number Diff line change
Expand Up @@ -396,10 +396,10 @@ class MockNativeChannel implements SentryNativeBinding {
class MockRendererWrapper implements RendererWrapper {
MockRendererWrapper(this._renderer);

final FlutterRenderer _renderer;
final FlutterRenderer? _renderer;

@override
FlutterRenderer getRenderer() {
FlutterRenderer? getRenderer() {
return _renderer;
}
}
Expand Down
34 changes: 7 additions & 27 deletions flutter/test/sentry_flutter_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -495,7 +495,7 @@ void main() {
await Sentry.close();
});

test('installed with skia renderer', () async {
test('installed on io platforms', () async {
List<Integration> integrations = [];

await SentryFlutter.init(
Expand All @@ -505,7 +505,8 @@ void main() {
integrations = options.integrations;
},
appRunner: appRunner,
platformChecker: getPlatformChecker(platform: MockPlatform.iOs()),
platformChecker:
getPlatformChecker(platform: MockPlatform.iOs(), isWeb: false),
rendererWrapper: MockRendererWrapper(FlutterRenderer.skia),
);

Expand All @@ -528,7 +529,8 @@ void main() {
integrations = options.integrations;
},
appRunner: appRunner,
platformChecker: getPlatformChecker(platform: MockPlatform.iOs()),
platformChecker:
getPlatformChecker(platform: MockPlatform.iOs(), isWeb: true),
rendererWrapper: MockRendererWrapper(FlutterRenderer.canvasKit),
);

Expand All @@ -551,7 +553,8 @@ void main() {
integrations = options.integrations;
},
appRunner: appRunner,
platformChecker: getPlatformChecker(platform: MockPlatform.iOs()),
platformChecker:
getPlatformChecker(platform: MockPlatform.iOs(), isWeb: true),
rendererWrapper: MockRendererWrapper(FlutterRenderer.html),
);

Expand All @@ -563,29 +566,6 @@ void main() {

await Sentry.close();
}, testOn: 'vm');

test('not installed with unknown renderer', () async {
List<Integration> integrations = [];

await SentryFlutter.init(
(options) async {
options.dsn = fakeDsn;
options.automatedTestMode = true;
integrations = options.integrations;
},
appRunner: appRunner,
platformChecker: getPlatformChecker(platform: MockPlatform.iOs()),
rendererWrapper: MockRendererWrapper(FlutterRenderer.unknown),
);

expect(
integrations
.map((e) => e.runtimeType)
.contains(ScreenshotIntegration),
false);

await Sentry.close();
}, testOn: 'vm');
});

group('initial values', () {
Expand Down