-
Notifications
You must be signed in to change notification settings - Fork 28.2k
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
integration_test example Android app: migrate to Gradle KTS #157193
integration_test example Android app: migrate to Gradle KTS #157193
Conversation
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
(PR triage): Hey @bartekpacia thanks for contributing. It looks like there are some failing checks here. |
f5271b6
to
082ec44
Compare
@reidbaker @gmackall do you have an idea why e.g.
|
Updating to master and rerunning to see if the failure repetes. Looking at the past 100 or so runs I saw 2 failures but I think they were on pull requests that modified the integration test code. |
It looks like the analyzer is complaining about a kotlin lint:
|
c787d7a
to
84d4c41
Compare
pluginManagement {
val flutterSdkPath = run {
val properties = java.util.Properties()
file("local.properties").inputStream().use { properties.load(it) }
val flutterSdkPath = properties.getProperty("flutter.sdk")
require(flutterSdkPath != null) { "flutter.sdk not set in local.properties" }
flutterSdkPath
}
// ...
} to look like this: pluginManagement {
val flutterSdkPath =
run {
val properties = java.util.Properties()
file("local.properties").inputStream().use { properties.load(it) }
val flutterSdkPath = properties.getProperty("flutter.sdk")
require(flutterSdkPath != null) { "flutter.sdk not set in local.properties" }
flutterSdkPath
}
// ...
} which (1) I really don't like but more importantly (2) the first version has been the default for quite some time now, so I don't think we should change it. Could we maybe disable ktlint for |
84d4c41
to
256e5d1
Compare
@gmackall what do you think. In general I dont like debating style and lean towards any automatic formatting. This reason alone isnt enough for me to want to disable it for all kts files but I could be convinced otherwise. |
I am inclined to agree that unless there is a very strong argument against a specific auto formatting case, we should just accept the auto formatting wholesale (including here) |
This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
(triage) @bartekpacia Do you still have plans for this PR? |
Yes, I need to figure out why the tests are failing (and I've got no idea why as of now) |
7c41359
to
21cc7c4
Compare
21cc7c4
to
e2f96d6
Compare
@bartekpacia Do you still have plans to return to this PR? |
I would like to but I have 0 idea why it fails. It's probably some little thing but I don't notice it. |
e2f96d6
to
9df9cfd
Compare
9df9cfd
to
dffd0cf
Compare
dffd0cf
to
206b31f
Compare
wooo all green :) ("it fixed itself" lol) |
After this PR is merged, all of
integration_test
Android gradle buildscripts will be in Kotlin.Follow-up of #156291
Part of ongoing effort to use Gradle Kotlin DSL a bit more
Pre-launch Checklist
///
).