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

Change PluginClasspath for tests to binary plugin from maven local #613

Merged
merged 5 commits into from
Oct 21, 2022

Conversation

rougsig
Copy link
Collaborator

@rougsig rougsig commented Sep 30, 2022

Background
Plugin can't works in test with classes that were loaded by gradle project.

For example:
Our plugin works with classes from agp. Agp is not bundled into plugin.
Plugin checks is agp was applied, but for some reasons ClassNotFoundException will be thrown. BaseExtension not found.

withPlugin("android") {
  // if we are here it means gradle has successfully loaded agp plugin
  project.android as BaseExtension
}

Changes
Remove withPluginClasspath from tests. Provide the plugin under test via maven local. Before the test run, gradle will publish the plugin to maven local.

Test plan
Green pipelines.

Source of the problem
withPluginClasspath loads the plugin under test to classloader_1. Gradle loads applied plugins to classpath_2. classpath_1 is the parent to classpath_2. The parent classpath cannot work with classes loaded by child classpaths.

References
gradle/gradle#11338
https://discuss.gradle.org/t/testkit-somehow-excludes-compileonly-dependencies/19842/5

@ejona86
Copy link
Collaborator

ejona86 commented Sep 30, 2022

We'll probably want to do a release before we do this. In particular, upgrading gradle-plugin-publish changes semantics in ways you have to carefully read the release notes to catch.

But I also think we should just do a release now-ish. I think #612 is the main thing that'd cause us delay. But even that, I'd be okay with a patch release.

@rougsig
Copy link
Collaborator Author

rougsig commented Sep 30, 2022

Yes of course.

I tried to run android functional tests. But this issue blocks me gradle/gradle#11338 (comment), https://discuss.gradle.org/t/testkit-somehow-excludes-compileonly-dependencies/19842/5, trying for workaround.

TLTR: for some reasons gradle test runner doesn't include compileOnly to test classpath.

I created a draft to run tests, my current PC doesn't have enough RAM to run android tests.

@ejona86
Copy link
Collaborator

ejona86 commented Oct 1, 2022

for some reasons gradle test runner doesn't include compileOnly to test classpath.

Wouldn't that need to be testCompileOnly? And I'd never expect the test runner to use it, as that is runtime; I'd only expect javac to use it.

I created a draft to run tests, my current PC doesn't have enough RAM to run android tests.

I figured it was something like that. It runs a lot faster in the CI with running each test in parallel. I had just noticed you were working on it and gave a peek.

@ejona86
Copy link
Collaborator

ejona86 commented Oct 1, 2022

I created a draft to run tests, my current PC doesn't have enough RAM to run android tests.

FWIW, you can also create PRs on your own personal fork, and I think Github Actions will still work there without any real issues (although I can't say I've actually done it for protobuf-gradle-plugin since we swapped to github actions). (I used to do that all the time for Travis-CI while I was debugging Travis-specific problems and trying new config.)

@rougsig rougsig changed the title Bump library versions & gradle clean up Change PluginClasspath for tests to binary plugin from maven local Oct 1, 2022
settings.gradle Outdated Show resolved Hide resolved
@rougsig
Copy link
Collaborator Author

rougsig commented Oct 4, 2022

But I also think we should just do a release now-ish. I think #612 is the main thing that'd cause us delay. But even that, I'd be okay with a patch release.

I think we can create patch release after #612. Also we need to remove/replace idea plugin, via to gradle/gradle#22233.

About release. I think we can merge #616 and do a release.

@rougsig rougsig marked this pull request as ready for review October 4, 2022 18:38
@rougsig rougsig requested a review from ejona86 October 4, 2022 18:38
@rougsig rougsig marked this pull request as draft October 4, 2022 20:09
@rougsig rougsig marked this pull request as ready for review October 6, 2022 20:03
@rougsig rougsig requested a review from ejona86 October 16, 2022 20:22
@rougsig rougsig merged commit f510de7 into google:master Oct 21, 2022
@rougsig rougsig deleted the android-gradle-testkit branch October 23, 2022 19:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants