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

Java: Add Android Gradle Plugin 8 and Spring Boot 3 tests #16336

Merged
merged 5 commits into from May 15, 2024

Conversation

smowton
Copy link
Contributor

@smowton smowton commented Apr 26, 2024

No description provided.

@smowton smowton requested a review from a team as a code owner April 26, 2024 10:59
@github-actions github-actions bot added the Java label Apr 26, 2024
@smowton smowton force-pushed the smowton/admin/android-8-test branch 2 times, most recently from f5e6951 to beb3cbc Compare April 29, 2024 14:44
Copy link
Member

@mbg mbg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed this as part of the associated, internal PR. Nice to have some integration tests for this! Couple of questions:

  • The two new integration tests include the .gradle folder. Is that intentional? If so why? Other integration tests that use Gradle don't seem to do this.
  • It may be useful to add a comment before the use of try_use_java11() to note why this is necessary. Particularly, it's not immediately obvious what impact this has on the version selection logic and how it affects the test outcome when compared to what may be happening in users' workflows.

import sys
import tempfile

def actions_expose_all_toolchains():
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a really nice addition for testing!

@smowton smowton force-pushed the smowton/admin/android-8-test branch from beb3cbc to 63fa719 Compare May 10, 2024 17:13
Copy link
Member

@mbg mbg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing my comments. This seems fine as is now, with one minor suggestion for improvement, but up to you if you want to act on it here / in a different PR / at all.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: bit odd to have such a comprehensive .gitignore specifically for this test. Consider moving it up to the most distant ancestor of this directory where it makes sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These things are generated by Gradle wrapper installation-- you can see quite a few in the test directories for this reason

@smowton smowton force-pushed the smowton/admin/android-8-test branch from 4d8e9c1 to bfb291c Compare May 14, 2024 18:25
Copy link
Contributor

QHelp previews:

@smowton smowton force-pushed the smowton/admin/android-8-test branch from bfb291c to 942df38 Compare May 15, 2024 10:19
@smowton smowton force-pushed the smowton/admin/android-8-test branch from 942df38 to a50584c Compare May 15, 2024 13:52
@smowton smowton merged commit 4617c05 into github:main May 15, 2024
9 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants