null safety on sentry_flutter #337

fzyzcjy · 2021-03-05T14:16:41Z

📜 Description

add support for null safety on sentry_flutter as shown here #247

💡 Motivation and Context

everyone who wants to have sound null safety needs this!

💚 How did you test it?

same tests, no need for any changes

📝 Checklist

🔮 Next steps

marandaneto · 2021-03-05T14:22:22Z

thanks, @fzyzcjy I will check this asap
@ueman would you be willing to review this too as you have done the whole sentry package? :D

ueman · 2021-03-05T14:23:31Z

Yeah sure

ueman · 2021-03-05T14:33:45Z

@fzyzcjy
I'm pretty sure, that some (most?) tests need to be migrated away from mockito because it can't really handle null values.

See for example:
https://github.com/getsentry/sentry-dart/blob/e2077a1241c5832e27d38f84904f3d6abbfec320/dart/test/hub_test.dart and
https://github.com/getsentry/sentry-dart/blob/e2077a1241c5832e27d38f84904f3d6abbfec320/dart/test/mocks/mock_hub.dart
on how it's done in the sentry package.

I wonder if we can share those mock between sentry and sentry_flutter.

I also think that you could remove most getter and setter in SentryFlutterOptions because the null checks aren't needed anymore.

fzyzcjy · 2021-03-05T14:45:51Z

@ueman sounds reasonable! I will check them after about 10 hours (after a sleep).

kuhnroyal · 2021-03-05T14:51:43Z

FYI : https://github.com/dart-lang/mockito/blob/master/NULL_SAFETY_README.md

ueman · 2021-03-05T15:14:30Z

FYI : https://github.com/dart-lang/mockito/blob/master/NULL_SAFETY_README.md

I know. But as per https://develop.sentry.dev/sdk/philosophy/#dependencies-cost I've decided against it. Also Flutter itself and a lot of other popular libraries are migrating away from it so I'm not that convinced 😅

flutter/lib/src/navigation/sentry_navigator_observer.dart

flutter/lib/src/widgets_binding_observer.dart

ueman · 2021-03-05T15:26:59Z

flutter/pubspec.yaml

@@ -5,7 +5,7 @@ homepage: https://docs.sentry.io/platforms/flutter/
 repository: https://github.com/getsentry/sentry-dart

 environment:
-  sdk: ">=2.8.0 <3.0.0"
+  sdk: '>=2.12.0 <3.0.0'
  flutter: ">=1.17.0"


@marandaneto Does it make sense to keep it this low? I wonder if it causes any problems because Flutter 1.17 doesn't has null safety. On the other hand
package_info for example allows versions as low as 1.12 while also having null safety.

yeah unless we have to, lets keep as it is, all the official plugins are down to 1.12

btw looking at https://dart.dev/null-safety

Sound null safety is available in Dart 2.12 and Flutter 2.

should we bump to min. v2?

flutter/test/load_android_image_list_test.dart

kuhnroyal · 2021-03-05T16:09:14Z

I understand your point but you could always add a sub project for tests that doesn't increase the SDK dependency surface and then just generate the mocks.

bruno-garcia · 2021-03-05T21:41:48Z

FYI : https://github.com/dart-lang/mockito/blob/master/NULL_SAFETY_README.md

I know. But as per https://develop.sentry.dev/sdk/philosophy/#dependencies-cost I've decided against it. Also Flutter itself and a lot of other popular libraries are migrating away from it so I'm not that convinced 😅

Worth noting that document refers to runtime dependencies and not dev or test only ones.

fzyzcjy · 2021-03-06T00:46:43Z

@ueman most of the reviews are fixed. now I am looking at the tests.

fzyzcjy · 2021-03-06T01:27:09Z

I also think that you could remove most getter and setter in SentryFlutterOptions because the null checks aren't needed anymore.

Done

fzyzcjy · 2021-03-06T01:32:16Z

Hi, I wonder what should I do to migrate the tests? Shall I use mockito, or not use it? @bruno-garcia @kuhnroyal @ueman

fzyzcjy · 2021-03-06T06:09:37Z

So I will start working on mockito NULL_SAFETY_README

codecov-io · 2021-03-06T06:31:09Z

Codecov Report

Merging #337 (a443a43) into feat/null-safety (e2077a1) will increase coverage by 1.14%.
The diff coverage is n/a.

@@                 Coverage Diff                  @@
##           feat/null-safety     #337      +/-   ##
====================================================
+ Coverage             88.41%   89.56%   +1.14%     
====================================================
  Files                    51       46       -5     
  Lines                  1692     1466     -226     
====================================================
- Hits                   1496     1313     -183     
+ Misses                  196      153      -43

Impacted Files	Coverage Δ
flutter/lib/src/file_system_transport.dart

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e2077a1...68458a8. Read the comment docs.

marandaneto · 2021-03-06T08:09:46Z

@fzyzcjy the package score is as low as 70, could you run pana https://pub.dev/packages/pana and see why its so low? maybe this action is running on an old pana version, not sure

flutter/lib/src/default_integrations.dart

flutter/lib/src/sentry_flutter.dart

flutter/test/mocks.mocks.dart

marandaneto · 2021-03-06T08:32:06Z

my 2 cents about using or not mockito with auto-generated code, I've not used mockito v5 yet (with null-safety enabled), so I don't have a strong opinion, as the tests are passing/fixed and right now null-safety at sentry is a blocker for some users, I'd prefer to go ahead and merge/release and later, we revisit this if necessary.

what do you think @ueman ?

ueman · 2021-03-06T08:36:07Z

my 2 cents about using or not mockito with auto-generated code, I've not used mockito v5 yet (with null-safety enabled), so I don't have a strong opinion, as the tests are passing/fixed and right now null-safety at sentry is a blocker for some users, I'd prefer to go ahead and merge/release and later, we revisit this if necessary.

what do you think @ueman ?

I agree with you.

flutter/pubspec.yaml

fzyzcjy · 2021-03-06T09:40:43Z

@fzyzcjy the package score is as low as 70, could you run pana https://pub.dev/packages/pana and see why its so low? maybe this action is running on an old pana version, not sure

@marandaneto Here is the outputs:

(base) ➜ flutter git:(feat/null-safety) pana
INFO Running /Users/tom/opt/flutter/bin/cache/dart-sdk/bin/dart --version...
INFO Running flutter --no-version-check --version --machine...
WARNING pana might update or modify files in ..
Analysis will begin in 15 seconds, hit CTRL+C to abort it.
To remove this message, use --no-warning.

INFO Running flutter --no-version-check packages pub upgrade --verbosity io --no-precompile...
SEVERE Problem with pub upgrade
Bad state: For sentry, the parsed version null did not match the locked version 4.0.6.
#0 _validateLockedVersions. (package:pana/src/pkg_resolution.dart:224:13)
#1 SplayTreeMap.forEach (dart:collection/splay_tree.dart:437:8)
#2 _validateLockedVersions (package:pana/src/pkg_resolution.dart:220:14)
#3 createPkgResolution (package:pana/src/pkg_resolution.dart:205:5)
#4 PackageContext.resolveDependencies (package:pana/src/package_context.dart:66:26)

#5 PackageAnalyzer._inspect (package:pana/src/package_analyzer.dart:121:27)

#6 main (file:///Users/tom/.pub-cache/hosted/pub.flutter-io.cn/pana-0.15.3/bin/pana.dart:163:19)

INFO Analyzing package...
INFO Running /Users/tom/opt/flutter/bin/cache/dart-sdk/bin/dartanalyzer --options /Users/tom/RefCode/sentry-dart/flutter/pana_analysis_options_1615023530394614.g.yaml --format machine lib...
INFO Running /Users/tom/opt/flutter/bin/cache/dart-sdk/bin/pub get...
INFO Running /Users/tom/opt/flutter/bin/cache/dart-sdk/bin/pub outdated --json --up-to-date --no-dev-dependencies --no-dependency-overrides...

✓ Follow Dart file conventions (20 / 20)

[*] 10/10 points: Provide a valid `pubspec.yaml`

[*] 5/5 points: Provide a valid `README.md`

[*] 5/5 points: Provide a valid `CHANGELOG.md`

✓ Provide documentation (10 / 10)

[*] 10/10 points: Package has an example

Found example at: example/lib/main.dart

✓ Support multiple platforms (20 / 20)

[*] 20/20 points: Supports 3 of 3 possible platforms (iOS, Android, Web)

Found 6 issues. Showing the first 2:

Consider supporting these prerelease platforms:

Package does not support Flutter platform Windows

Because:

package:sentry_flutter/sentry_flutter.dart that declares support for platforms: Android, iOS, Web

✗ Pass static analysis (0 / 30)

[x] 0/30 points: code has no errors, warnings, lints, or formatting issues

Found 7 issues. Showing the first 2:

ERROR: The symbol 'RunZonedGuardedIntegration' is defined in a legacy library, and can't be re-exported from a library with null safety enabled.

lib/sentry_flutter.dart:2:8

  ╷
2 │ export 'package:sentry/sentry.dart';
  │        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  ╵

To reproduce make sure you are using pedantic and run flutter analyze lib/sentry_flutter.dart

INFO: The library 'package:sentry/sentry.dart' is legacy, and should not be imported into a null safe library.

lib/src/default_integrations.dart:7:8

  ╷
7 │ import 'package:sentry/sentry.dart';
  │        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  ╵

To reproduce make sure you are using pedantic and run flutter analyze lib/src/default_integrations.dart

✓ Support up-to-date dependencies (20 / 20)

[*] 10/10 points: All of the package dependencies are supported in the latest version

Package	Constraint	Compatible	Latest
`flutter`	`flutter`	0.0.0	0.0.0
`flutter_web_plugins`	`flutter`	0.0.0	0.0.0
`package_info`	`^2.0.0`	2.0.0	2.0.0
`sentry`	`^4.0.5`	4.0.6	4.0.6

Transitive dependencies

Package	Constraint	Compatible	Latest
`characters`	-	1.1.0	1.1.0
`charcode`	-	1.2.0	1.2.0
`collection`	-	1.15.0	1.15.0
`convert`	-	2.1.1	3.0.0
`crypto`	-	2.1.5	3.0.0
`http`	-	0.12.2	0.13.0
`http_parser`	-	3.1.4	4.0.0
`js`	-	0.6.3	0.6.3
`meta`	-	1.3.0	1.3.0
`path`	-	1.8.0	1.8.0
`sky_engine`	-	0.0.99	0.0.99
`source_span`	-	1.8.1	1.8.1
`stack_trace`	-	1.10.0	1.10.0
`string_scanner`	-	1.1.0	1.1.0
`term_glyph`	-	1.2.0	1.2.0
`typed_data`	-	1.3.0	1.3.0
`uuid`	-	2.2.2	3.0.1
`vector_math`	-	2.1.0	2.1.0

To reproduce run pub outdated --no-dev-dependencies --up-to-date --no-dependency-overrides.

[*] 10/10 points: Package supports latest stable Dart and Flutter SDKs

✓ Support sound null-safety (0 / 0)

[*] 0/0 points: Package and dependencies are fully migrated to null-safety, and will be awarded additional points in a planned future revision of the pub.dev points model.

Points: 70/100.

fzyzcjy · 2021-03-06T09:42:03Z

Thus, IMHO the problem is caused by, pana does not understand we need to use the overrided version of sentry, but use the old version on Pub? So I guess it is not a problem

fzyzcjy · 2021-03-07T00:44:51Z

Hi, can we merge this now? 😄

marandaneto

LGTM

fzyzcjy added 2 commits March 5, 2021 22:09

migrate to null safety for sentry_flutter

13eb5bf

make it compile on dart2.12/flutter2.0

697f41c

fzyzcjy requested review from bruno-garcia and marandaneto as code owners March 5, 2021 14:16

fzyzcjy mentioned this pull request Mar 5, 2021

Add support to null safety #247

Closed

7 tasks

marandaneto requested a review from ueman March 5, 2021 14:21

ueman reviewed Mar 5, 2021

View reviewed changes

fzyzcjy added 3 commits March 6, 2021 08:43

fix a few constructors

5cb2538

avoid WidgetsBinding.instance! as it can be null

07fcaee

remove as FutureOr Function()

b7b8e23

fzyzcjy added 2 commits March 6, 2021 09:22

fix compile error

5aeebd6

remove getter/setter which are only used for null checks

8550c13

fixbug some warnings

9c3ccff

fzyzcjy added 2 commits March 6, 2021 14:23

generate mocks using mockito code generator

a443a43

fix bugs about "!" on null value, and return null value

68458a8

fzyzcjy added 2 commits March 6, 2021 14:32

add a "when", otherwise mockito complains no stub value to return

20a9323

make _observer nullable, since call may not be called

ad05773

fzyzcjy added 3 commits March 6, 2021 14:40

add changelog

10247d1

execute flutter format . on sentry-flutter

85b76b0

fix analyze problems

fb2913a

marandaneto mentioned this pull request Mar 6, 2021

Upgrade dependencies (http 0.13.0 etc) for Flutter 2 upgrade #339

Closed

marandaneto reviewed Mar 6, 2021

View reviewed changes

flutter/lib/src/default_integrations.dart Outdated Show resolved Hide resolved

marandaneto reviewed Mar 6, 2021

View reviewed changes

flutter/lib/src/sentry_flutter.dart Show resolved Hide resolved

marandaneto reviewed Mar 6, 2021

View reviewed changes

flutter/test/mocks.mocks.dart Show resolved Hide resolved

ueman reviewed Mar 6, 2021

View reviewed changes

flutter/pubspec.yaml Outdated Show resolved Hide resolved

fzyzcjy added 2 commits March 6, 2021 17:46

change mockito version

adb9c4a

change signature of ensureInitialized

e3d8051

marandaneto approved these changes Mar 7, 2021

View reviewed changes

marandaneto merged commit 32d09c8 into getsentry:feat/null-safety Mar 7, 2021

Uh oh!

null safety on sentry_flutter #337

null safety on sentry_flutter #337

Uh oh!

Conversation

fzyzcjy commented Mar 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

Uh oh!

marandaneto commented Mar 5, 2021

Uh oh!

ueman commented Mar 5, 2021

Uh oh!

ueman commented Mar 5, 2021

Uh oh!

fzyzcjy commented Mar 5, 2021

Uh oh!

kuhnroyal commented Mar 5, 2021

Uh oh!

ueman commented Mar 5, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ueman Mar 5, 2021

Choose a reason for hiding this comment

Uh oh!

marandaneto Mar 6, 2021

Choose a reason for hiding this comment

Uh oh!

marandaneto Mar 6, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kuhnroyal commented Mar 5, 2021

Uh oh!

bruno-garcia commented Mar 5, 2021

Uh oh!

fzyzcjy commented Mar 6, 2021

Uh oh!

fzyzcjy commented Mar 6, 2021

Uh oh!

fzyzcjy commented Mar 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fzyzcjy commented Mar 6, 2021

Uh oh!

codecov-io commented Mar 6, 2021

Codecov Report

Uh oh!

marandaneto commented Mar 6, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

marandaneto commented Mar 6, 2021

Uh oh!

ueman commented Mar 6, 2021

Uh oh!

Uh oh!

fzyzcjy commented Mar 6, 2021

✓ Follow Dart file conventions (20 / 20)

[*] 10/10 points: Provide a valid pubspec.yaml

[*] 5/5 points: Provide a valid README.md

[*] 5/5 points: Provide a valid CHANGELOG.md

✓ Provide documentation (10 / 10)

[*] 10/10 points: Package has an example

✓ Support multiple platforms (20 / 20)

[*] 20/20 points: Supports 3 of 3 possible platforms (iOS, Android, Web)

✗ Pass static analysis (0 / 30)

[x] 0/30 points: code has no errors, warnings, lints, or formatting issues

✓ Support up-to-date dependencies (20 / 20)

[*] 10/10 points: All of the package dependencies are supported in the latest version

fzyzcjy commented Mar 5, 2021 •

edited

Loading

fzyzcjy commented Mar 6, 2021 •

edited

Loading

[*] 10/10 points: Provide a valid `pubspec.yaml`

[*] 5/5 points: Provide a valid `README.md`

[*] 5/5 points: Provide a valid `CHANGELOG.md`