-
-
Notifications
You must be signed in to change notification settings - Fork 248
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
Add Flutter runtime information #2742
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2742 +/- ##
==========================================
- Coverage 88.97% 88.96% -0.01%
==========================================
Files 263 263
Lines 8925 8926 +1
==========================================
Hits 7941 7941
- Misses 984 985 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ohh nice! 🥳 |
/// The URL of the Git repository from which Flutter was obtained. | ||
static const String? gitUrl = bool.hasEnvironment('FLUTTER_GIT_URL') | ||
? String.fromEnvironment('FLUTTER_GIT_URL') | ||
: null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be included in a context somewhere?
This PR is ready to review now, but maybe we should wait with merging a little to see whether the changes are sticking this time in the Flutter framework |
@buenaflor The linked PR is sticking, so it should be safe to merge now. |
// This is included since [Platform.version](https://api.dart.dev/stable/dart-io/Platform/version.html) | ||
// is not included on web platforms. | ||
/// The Dart version used to compile the app. | ||
static const String? dartVersion = bool.hasEnvironment('FLUTTER_DART_VERSION') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's slightly different than Platform.version
:
Platform.version
:3.8.0-133.0.dev (dev) (Sun Feb 23 12:02:50 2025 -0800) on "macos_x64"
FlutterVersion.dartVersion
:3.8.0 (build 3.8.0-133.0.dev)
@denrase Are you able to do a review on this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! \o/ We just need to make sure to also add it in the html enricher, as well as make CI happy.
/// | ||
/// When this Flutter version was build from a fork, or when Flutter runs in a | ||
/// custom embedder, these values might be unreliable. | ||
abstract class FlutterVersion { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is code from your Flutter PR, would it also make sense to bring the corresponding tests over?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lookin' good, thank you!
@ueman The tests are failing on current dart versions and we have one analyser issue (formatting) left. Could we disable the tests for unsupported dart/flutter versions? ![]() |
Ahh, good point. I think we can wrap the tests in a group and use Edit: Nevermind, those test will never work since they're executed in the Dart test runner which will never have those dart defines available, since they're only available in a Flutter environment. Soooo, maybe we should just delete the tests again? |
@ueman Ah got it, moving them to |
Good idea! I haven't thought about that, but I could imagine that working. |
@denrase It works 🥳 Everything that's red is unrelated to what I've done, I think. |
@ueman Cool, i'll check out your branch and see that I fix what's causing the CI to fail. |
@ueman Think the skip condition needs to be inverted. Also we can ignore failing e2e tests, since the token is missing in your branch, which is also why they fail. |
@ueman Would you be so kind and sync the main of your fork with sentry? Just landed a fix for CI issues. After that, I'll merge those in here and then we are good to go. 🙇♂️ |
@buenaflor FYI, the integration e2e test is not running because we do not have the token available in forks. Maybe we should consider skipping the test in this case? |
qq, is this important to have in v8? maybe we can just add it to v9 directly |
@buenaflor This is not breaking anything, just adding additional info about Flutter runtime. Don't see a reason why this could not be merged directly to main. WDYT? |
@denrase nothing really wrong with the PR and adding it to v8 but I'm asking because I was thinking of making a |
FWIW, I don't think the changes in this PR need a beta release |
Please go ahead, there's nothing to add from my side |
📜 Description
Fun fact: This fixes the oldest currently open issue in this repo
💡 Motivation and Context
#416
💚 How did you test it?
I'm not entirely sure how to test this, since Dart defines aren't mockable.
You can test this manually by switching to the master channel of Flutter and building an app.
📝 Checklist
sendDefaultPii
is enabled🔮 Next steps
Wait until the Flutter PR is merged, then merge this.