[pigeon] Fix missed casting of not nullable Dart int to Kotlin long #3004
Conversation
tarrinneal
left a comment
tarrinneal
left a comment
There was a problem hiding this comment.
Could you expand your tests to include the casting of all types and remove the 'header' from the titles of both tests? At the moment the test is technically not appropriately named for the new casting line you added.
I think this was passing our integration tests already is concerning. We may to to expand them to cover this instance.
Overall a solid pr. Thank you.
This PR should definitely include an integration test that covers the failing case. Any fix for a runtime failure that we find going forward should include a corresponding integration test at the minimum (and if it's useful, relevant unit tests). |
|
@ycherniavskyi Are you planning on adding tests per the discussion above? |
|
@stuartmorgan yes sure, but I am waiting to merge #2997 to reabase over it and add tests then. |
It has now been merged! |
|
@tarrinneal great, so I am going to rebase and add tests today. |
ycherniavskyi
force-pushed
the
fix-casting-dart-int-to-kotlin-long
branch
from
c9ba455 to
b1b7f1c
Compare
|
@tarrinneal rebased and consolidated unit tests with properties naming style seen in #3066. As for the integration tests, then, it seems they were already added within #3066, but disabled for Android Kotlin implementation. This PR resole the flutter/flutter#115914 that was mentioned in flutter/flutter#118726, which is the reason for some tests skipped for Android Kotlin implementation. So this PR for sure affords to remove some of this skip. |
Have you removed the relevant skips and verified locally that the tests now pass? If so, that change should be in this PR. |
# Conflicts: # packages/pigeon/CHANGELOG.md # packages/pigeon/lib/generator_tools.dart # packages/pigeon/pubspec.yaml
|
@stuartmorgan do it in 82ab8e8. Also, merge the last main and update gen files. |
Can you run the formatter cl tool with
to clean up the generated files and upload them again. |
By executing: dart pub global run flutter_plugin_tools format --packages pigeon
|
@tarrinneal done. |
tarrinneal
added
the
autosubmit
auto-submit
bot
removed
the
autosubmit
|
auto label is removed for flutter/packages, pr: 3004, due to - Please get at least one approved review if you are already a member or two member reviews if you are not a member before re-applying this label. Reviewers: If you left a comment approving, please use the "approve" review action instead. |
|
auto label is removed for flutter/packages, pr: 3004, due to Validations Fail. |
|
stuartmorgan-g
added
the
autosubmit
3 participants
Kotlin generator creates source code that throws on executing because of missed necessary casting.
Pre-launch Checklist
dart format.)[shared_preferences]pubspec.yamlwith an appropriate new version according to the [pub versioning philosophy], or this PR is [exempt from version changes].CHANGELOG.mdto add a description of the change, [following repository CHANGELOG style].///).