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

integration_test example Android app: migrate to Gradle KTS #157193

Merged
merged 4 commits into from
Mar 18, 2025

Conversation

bartekpacia
Copy link
Member

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

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
  • I signed the [CLA].
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is [test-exempt].
  • I followed the [breaking change policy] and added [Data Driven Fixes] where supported.
  • All existing and new tests are passing.

@flutter-dashboard
Copy link

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.

@github-actions github-actions bot added a: tests "flutter test", flutter_test, or one of our tests framework flutter/packages/flutter repository. See also f: labels. f: integration_test The flutter/packages/integration_test plugin labels Oct 18, 2024
@Piinks Piinks requested a review from gmackall October 22, 2024 22:15
@Piinks
Copy link
Contributor

Piinks commented Oct 22, 2024

(PR triage): Hey @bartekpacia thanks for contributing. It looks like there are some failing checks here.

@bartekpacia bartekpacia force-pushed the integration_test_full_on_kts branch from f5271b6 to 082ec44 Compare November 6, 2024 16:18
@bartekpacia
Copy link
Member Author

@reidbaker @gmackall do you have an idea why e.g. Mac_arm64 framework_tests_misc is failing? I run the equivalent command locally and it passes:

$ packages/integration_test/example/android
$ flutter build apk --debug
$ ./gradlew :integration_test:testDebugUnitTest --tests dev.flutter.plugins.integration_test.FlutterDeviceScreenshotTest

@reidbaker
Copy link
Contributor

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.

@Piinks
Copy link
Contributor

Piinks commented Nov 19, 2024

It looks like the analyzer is complaining about a kotlin lint:

 Summary error count (descending) by rule:
║   standard:multiline-expression-wrapping: 1

@bartekpacia
Copy link
Member Author

bartekpacia commented Nov 23, 2024

ktlint wants this block of code:

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 .kts files?

@bartekpacia bartekpacia force-pushed the integration_test_full_on_kts branch from 84d4c41 to 256e5d1 Compare November 23, 2024 21:02
@reidbaker
Copy link
Contributor

ktlint wants this block of code:

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 .kts files?

@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.

@gmackall
Copy link
Member

gmackall commented Dec 9, 2024

ktlint wants this block of code:

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 .kts files?

@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)

@flutter-dashboard
Copy link

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 package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@goderbauer
Copy link
Member

(triage) @bartekpacia Do you still have plans for this PR?

@bartekpacia
Copy link
Member Author

Yes, I need to figure out why the tests are failing (and I've got no idea why as of now)

@bartekpacia bartekpacia force-pushed the integration_test_full_on_kts branch 6 times, most recently from 7c41359 to 21cc7c4 Compare January 10, 2025 17:14
@bartekpacia bartekpacia force-pushed the integration_test_full_on_kts branch from 21cc7c4 to e2f96d6 Compare January 23, 2025 17:38
@justinmc
Copy link
Contributor

@bartekpacia Do you still have plans to return to this PR?

@bartekpacia
Copy link
Member Author

I would like to but I have 0 idea why it fails. It's probably some little thing but I don't notice it.

@bartekpacia bartekpacia force-pushed the integration_test_full_on_kts branch from e2f96d6 to 9df9cfd Compare March 14, 2025 23:59
@bartekpacia bartekpacia requested a review from matanlurey as a code owner March 14, 2025 23:59
@bartekpacia bartekpacia force-pushed the integration_test_full_on_kts branch from 9df9cfd to dffd0cf Compare March 15, 2025 00:02
@bartekpacia bartekpacia force-pushed the integration_test_full_on_kts branch from dffd0cf to 206b31f Compare March 15, 2025 00:12
@bartekpacia
Copy link
Member Author

wooo all green :)

("it fixed itself" lol)

cc @reidbaker @gmackall

@bartekpacia bartekpacia requested a review from reidbaker March 18, 2025 09:29
@reidbaker reidbaker added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 18, 2025
@auto-submit auto-submit bot added this pull request to the merge queue Mar 18, 2025
Merged via the queue into flutter:master with commit 7efef42 Mar 18, 2025
74 checks passed
@flutter-dashboard flutter-dashboard bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Mar 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a: tests "flutter test", flutter_test, or one of our tests f: integration_test The flutter/packages/integration_test plugin framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants