Add migration gotchas and clarify external plugin usage#353
Add migration gotchas and clarify external plugin usage#353bulldozer-bot[bot] merged 4 commits intodevelopfrom
Conversation
felixdesouza
force-pushed
the
docs/migration-gotchas
branch
from
cfd757a to
af80770
Compare
- Clarify that `gradlePluginForTesting` configuration is already created by the plugin - Add "Common Mistake" callout showing wrong pattern (buildscript blocks) vs correct pattern - Add "Common Migration Gotchas" section covering: - Don't access framework internals (no need for version-based namespacing) - Use full task paths for explicit task assertions 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
felixdesouza
force-pushed
the
docs/migration-gotchas
branch
from
af80770 to
b7484a0
Compare
The gradle-plugin-testing plugin automatically provides the correct version of this dependency, so it should not be added to versions.props. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
docs/testing-guide.md
Outdated
|
|
||
| ### Common Migration Gotchas | ||
|
|
||
| When migrating from Nebula/Spock tests to this framework, avoid these common mistakes: |
There was a problem hiding this comment.
should this live in the junit-migration-excavator-instructions file instead?
There was a problem hiding this comment.
maybe? feels more AI focused so maybe should be moved?
There was a problem hiding this comment.
Yeah, I don't think this file should reference migrations. Hopefully people can just read it without having to worry about groovy based tests at some point.
docs/testing-guide.md
Outdated
|
|
||
| ```java | ||
| // Preferred - chain the calls | ||
| rootProject.buildGradle().plugins().add("com.palantir.failure-reports").add("java"); |
There was a problem hiding this comment.
Is this better? I think it's fine if the .adds are on multiple lines, but I think the .adds on the same line is less readable.
There was a problem hiding this comment.
that's fair, but it's also up to formatting. I wonder whether a varargs thing would be better
docs/testing-guide.md
Outdated
|
|
||
| ```java | ||
| // WRONG - Accessing implementation details | ||
| String version = ((DefaultGradleInvoker) gradle).gradleVersion().version(); |
There was a problem hiding this comment.
Maybe we should be more liberal with @RestrictedApi too if these need to be public. I kinda hate it but we probably just have put everything in same package.
There was a problem hiding this comment.
for restricted api absolutely, there's no downside and it guides the ai more I believe?
Refactor documentation to separate general usage guidance from migration-specific advice. Migration gotchas now live in junit-migration-excavator-instructions.md alongside other migration guidance. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
3 participants
Before this PR
The testing guide's "Testing with External Plugins" section doesn't clarify that
gradlePluginForTestingis already created by the plugin, and the example uses an explicit version number.Migration-specific gotchas were scattered or missing from documentation.
After this PR
testing-guide.md (general usage):
gradlePluginForTestingconfiguration is automatically created by the plugin.add()calls for pluginsjunit-migration-excavator-instructions.md (migration-specific):
gradle-plugin-testing-junittoversions.propsas the plugin provides it automaticallyBased on issues discovered during review of migration PR palantir/gradle-failure-reports#432
Possible downsides?
None - documentation only.