Fix Release process #3255
Fix Release process #3255
Conversation
Fixes testng-team#3254 Fixing the release process. Also added documentation to help fix the release process
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughReplaces the legacy OSSRH/stage-vote-release flow with nmcp-based Central Portal publishing, raises the default build JDK to 21, adds release/centralPortal/signing build parameters, updates CI workflows for JDK 21 and snapshot/manual publishing, upgrades Kotlin/Groovy/tooling, and adds extensive build/release/Java-version documentation and CONTRIBUTING guidance. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer / Maintainer
participant GHA as GitHub Actions
participant Gradle as Gradle (build logic / nmcp)
participant CP as Maven Central Portal
participant Nexus as Central Snapshots
rect rgb(223,242,225)
Note right of GHA: Publish flow (manual dispatch)
Dev->>GHA: Trigger `publish-maven-central` (AUTOMATIC / USER_MANAGED)
GHA->>GHA: Set up JDK 21
GHA->>Gradle: Run publishAggregationToCentralPortal\n(-Prelease, -PcentralPortal.publishingType)
Gradle->>Gradle: Load build params (release, centralPortal, signing)
Gradle->>Gradle: Evaluate signing (PGP from props/env)
Gradle->>CP: Authenticate (CENTRAL_PORTAL_USERNAME/PASSWORD)
Gradle->>CP: Upload artifacts (publish or stage)
alt AUTOMATIC
CP-->>GHA: Confirm published
GHA-->>Dev: Display published URL
else USER_MANAGED
CP-->>GHA: Confirm staged
GHA-->>Dev: Display portal review URL
end
end
rect rgb(255,243,214)
Note right of GHA: Snapshot flow (push -> job)
Dev->>GHA: Push -> `publish-snapshot` job
GHA->>GHA: Set up JDK 21
GHA->>Gradle: Run snapshot publish task
Gradle->>Gradle: Configure `centralSnapshots` when !release (creds from env)
Gradle->>Nexus: Publish snapshot
Nexus-->>GHA: Snapshot published
GHA-->>Dev: Display snapshot URL
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
docs/README.md (1)
115-127: Optional: Add language hint to code block for consistency.The code block representing the project structure could specify a language identifier (e.g.,
textorplaintext) for better markdown consistency, though it's purely a stylistic improvement.🔎 Suggested refinement
-``` +```text testng/ ├── build-logic/ # Composite build for build logic │ ├── build-parameters/ # Build parameter definitionsdocs/BUILD_SYSTEM.md (2)
26-38: Optional: Add language hints to diagram code blocks.The ASCII art diagrams would benefit from language identifiers (e.g.,
textorplaintext) for markdown consistency. This is a minor stylistic improvement.Also applies to: 94-111, 148-162, 201-234
263-263: Optional: Convert bare URL to markdown link.For consistency with markdown best practices, consider converting the bare URL to a markdown link format.
🔎 Suggested refinement
-- **USER_MANAGED**: Artifacts are staged in the portal; you manually publish from https://central.sonatype.com/publishing +- **USER_MANAGED**: Artifacts are staged in the portal; you manually publish from [Central Portal](https://central.sonatype.com/publishing)docs/CI_TEST_WORKFLOW.md (1)
35-72: Optional: Add language hints to workflow diagram code blocks.Similar to other documentation files, the ASCII art workflow diagrams could specify a language identifier (e.g.,
text) for markdown consistency. This is a minor stylistic improvement.Also applies to: 383-433, 452-490, 798-947
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
.github/CONTRIBUTING.md.github/workflows/publish-maven-central.yml.github/workflows/publish-snapshot.yml.github/workflows/release-maven-central.ymlREADME.mdbuild-logic/build-parameters/build.gradle.ktsbuild-logic/publishing/build.gradle.ktsbuild-logic/publishing/src/main/kotlin/testng.maven-publish.gradle.ktsbuild-logic/publishing/src/main/kotlin/testng.signing.gradle.ktsbuild.gradle.ktsdocs/BUILD_SYSTEM.mddocs/CI_TEST_WORKFLOW.mddocs/JAVA_VERSIONS_QUICK_REFERENCE.mddocs/README.mddocs/RELEASE_PROCESS.md
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-09-30T05:21:41.566Z
Learnt from: krmahadevan
Repo: testng-team/testng PR: 3181
File: testng-core/src/test/java/test/invocationcount/FailedInvocationCountTest.java:162-162
Timestamp: 2024-09-30T05:21:41.566Z
Learning: TestNG requires a minimum Java version of JDK 11.
Applied to files:
README.mddocs/JAVA_VERSIONS_QUICK_REFERENCE.md.github/CONTRIBUTING.md
🧬 Code graph analysis (1)
build-logic/publishing/build.gradle.kts (1)
build-logic/publishing/src/main/kotlin/buildlogic/OptionalFeaturesExtension.kt (1)
api(28-30)
🪛 LanguageTool
docs/RELEASE_PROCESS.md
[style] ~165-~165: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...or release and want extra caution - You want to coordinate the release timing #### Ste...
(REP_WANT_TO_VB)
[style] ~339-~339: Consider using “email”.
Context: ...e"** ### 3. Send Release Announcement Send an email to the TestNG users mailing list: To:...
(SEND_AN_EMAIL)
[style] ~412-~412: Consider an alternative verb to strengthen your wording.
Context: ...he workflow already uses JDK 17. If you see this error, check: - The workflow file...
(IF_YOU_HAVE_THIS_PROBLEM)
[uncategorized] ~656-~656: The official name of this software platform is spelled with a capital “H”.
Context: ...-----| | Publish to Maven Central | .github/workflows/publish-maven-central.yml | ...
(GITHUB)
[uncategorized] ~657-~657: The official name of this software platform is spelled with a capital “H”.
Context: ...e publishing | | Publish Snapshot | .github/workflows/publish-snapshot.yml | Autom...
(GITHUB)
[uncategorized] ~658-~658: The official name of this software platform is spelled with a capital “H”.
Context: ...ishing on push to master | | Test | .github/workflows/test.yml | Run tests on PRs ...
(GITHUB)
README.md
[uncategorized] ~12-~12: The official name of this software platform is spelled with a capital “H”.
Context: ...s JDK 17 or higher. See CONTRIBUTING.md for details. ### Rele...
(GITHUB)
docs/JAVA_VERSIONS_QUICK_REFERENCE.md
[style] ~192-~192: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ... Do I need to install Java 17 if I only want to run TestNG in my project? A: No! T...
(REP_WANT_TO_VB)
docs/CI_TEST_WORKFLOW.md
[uncategorized] ~3-~3: The official name of this software platform is spelled with a capital “H”.
Context: ... TestNG's GitHub Actions test workflow (.github/workflows/test.yml) works, including t...
(GITHUB)
[uncategorized] ~76-~76: The official name of this software platform is spelled with a capital “H”.
Context: ...he matrix builder is a JavaScript file (.github/workflows/matrix.js) that generates a ...
(GITHUB)
[uncategorized] ~565-~565: The official name of this software platform is spelled with a capital “H”.
Context: ...`` ### Adding a New Java Version Edit .github/workflows/matrix.js: ```javascript ma...
(GITHUB)
[uncategorized] ~585-~585: The official name of this software platform is spelled with a capital “H”.
Context: ...### Adding a New Operating System Edit .github/workflows/matrix.js: ```javascript ma...
(GITHUB)
docs/BUILD_SYSTEM.md
[uncategorized] ~269-~269: The official name of this software platform is spelled with a capital “H”.
Context: ...ion and publishing. ### Test Workflow (.github/workflows/test.yml) The test workflow...
(GITHUB)
[uncategorized] ~275-~275: The official name of this software platform is spelled with a capital “H”.
Context: ...nfiguration The matrix is generated by .github/workflows/matrix.js: - **Java version...
(GITHUB)
[uncategorized] ~314-~314: The official name of this software platform is spelled with a capital “H”.
Context: ...trix version ``` ### Publish Workflow (.github/workflows/publish-maven-central.yml) ...
(GITHUB)
[uncategorized] ~331-~331: The official name of this software platform is spelled with a capital “H”.
Context: ...ing_type }} ``` ### Snapshot Workflow (.github/workflows/publish-snapshot.yml) Autom...
(GITHUB)
🪛 markdownlint-cli2 (0.18.1)
docs/RELEASE_PROCESS.md
27-27: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
96-96: Bare URL used
(MD034, no-bare-urls)
97-97: Bare URL used
(MD034, no-bare-urls)
109-109: Bare URL used
(MD034, no-bare-urls)
139-139: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
148-148: Bare URL used
(MD034, no-bare-urls)
149-149: Bare URL used
(MD034, no-bare-urls)
169-169: Bare URL used
(MD034, no-bare-urls)
197-197: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
202-202: Bare URL used
(MD034, no-bare-urls)
230-230: Bare URL used
(MD034, no-bare-urls)
231-231: Bare URL used
(MD034, no-bare-urls)
325-325: Bare URL used
(MD034, no-bare-urls)
342-342: Bare URL used
(MD034, no-bare-urls)
348-348: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
428-428: Bare URL used
(MD034, no-bare-urls)
431-431: Bare URL used
(MD034, no-bare-urls)
452-452: Bare URL used
(MD034, no-bare-urls)
463-463: Bare URL used
(MD034, no-bare-urls)
464-464: Bare URL used
(MD034, no-bare-urls)
495-495: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
509-509: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
531-531: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
569-569: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
644-644: Bare URL used
(MD034, no-bare-urls)
645-645: Bare URL used
(MD034, no-bare-urls)
646-646: Bare URL used
(MD034, no-bare-urls)
647-647: Bare URL used
(MD034, no-bare-urls)
648-648: Bare URL used
(MD034, no-bare-urls)
649-649: Bare URL used
(MD034, no-bare-urls)
650-650: Bare URL used
(MD034, no-bare-urls)
677-677: Bare URL used
(MD034, no-bare-urls)
678-678: Bare URL used
(MD034, no-bare-urls)
docs/JAVA_VERSIONS_QUICK_REFERENCE.md
15-15: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
217-217: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
docs/CI_TEST_WORKFLOW.md
35-35: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
383-383: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
452-452: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
798-798: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
docs/README.md
115-115: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
.github/CONTRIBUTING.md
39-39: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
docs/BUILD_SYSTEM.md
26-26: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
94-94: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
148-148: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
201-201: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
263-263: Bare URL used
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: 17, temurin, ubuntu, UTC, tr_TR
- GitHub Check: 11, zulu, windows, UTC, de_DE
- GitHub Check: 17, microsoft, same hashcode, ubuntu, UTC, tr_TR
- GitHub Check: 21, temurin, macos, America/New_York, fr_FR
- GitHub Check: 17, microsoft, ubuntu, UTC, tr_TR, stress JIT
- GitHub Check: 21, microsoft, ubuntu, Pacific/Chatham, de_DE
🔇 Additional comments (15)
README.md (1)
8-12: LGTM!The requirements section clearly documents the Java version requirements for both users and contributors, aligning with the build system changes in this PR.
.github/CONTRIBUTING.md (1)
41-56: LGTM!The JDK requirements section provides clear guidance for contributors, explaining the difference between build requirements (JDK 17+) and runtime requirements (Java 11+), with helpful context about why this distinction exists.
docs/JAVA_VERSIONS_QUICK_REFERENCE.md (1)
1-231: LGTM!This comprehensive documentation effectively explains the multi-version Java build setup. The visual diagrams, scenarios, and FAQ section provide valuable guidance for both contributors and users.
The static analysis hints about missing language specifiers for code blocks (lines 15-39, 217-230) and style suggestions are optional nitpicks that don't affect the documentation's clarity or accuracy.
build-logic/publishing/src/main/kotlin/testng.signing.gradle.kts (2)
14-20: LGTM!The environment variable fallback for PGP credentials enhances CI/CD flexibility while maintaining support for project properties. The conditional initialization (only when both key and passphrase are available) prevents partial configuration errors.
22-25: LGTM!The conditional signing logic correctly gates signing based on the
signing.pgp.enabledparameter and release flag, allowing flexible control over when artifacts are signed (e.g., skip signing for snapshot builds).build-logic/publishing/src/main/kotlin/testng.maven-publish.gradle.kts (1)
54-73: LGTM!The conditional snapshot repository configuration correctly uses the
releasebuild parameter to determine when to configure the Central Snapshots repository. The environment variable-based credential handling aligns with the signing configuration and supports CI/CD workflows.docs/RELEASE_PROCESS.md (1)
1-685: LGTM!This comprehensive release process documentation effectively guides users through both AUTOMATIC and USER_MANAGED publishing workflows. The visual diagrams, step-by-step instructions, troubleshooting section, and comparison with the old process provide valuable context for the migration to the NMCP-based publishing system.
The static analysis hints about bare URLs and missing code block language specifiers are optional formatting improvements that don't affect the documentation's clarity or usefulness.
build-logic/build-parameters/build.gradle.kts (3)
56-59: LGTM!The
releaseparameter provides clear control over whether to create a release version, with an appropriate default offalseto prevent accidental releases.
60-66: LGTM!The
centralPortal.publishingTypeparameter clearly defines the two publishing modes (AUTOMATIC and USER_MANAGED) with AUTOMATIC as the recommended default, aligning with the release process documentation.
67-75: LGTM!The
signing.pgp.enabledparameter provides flexible control over artifact signing, with AUTO as the default to enable signing for release versions while allowing it to be disabled when needed..github/workflows/publish-maven-central.yml (1)
1-63: LGTM! Clean migration to Central Portal publishing.The workflow correctly:
- Uses manual dispatch with appropriate publishing type options
- Sets up JDK 17 for nmcp plugin requirements
- Secures credentials via GitHub secrets
- Provides clear completion messages for both publishing modes
The implementation aligns well with the PR objectives to replace the deprecated stage-vote-release workflow.
.github/workflows/publish-snapshot.yml (1)
1-41: LGTM! Clean snapshot publishing workflow.The workflow correctly:
- Triggers on pushes to master for automatic snapshot publishing
- Sets up JDK 17 for nmcp plugin compatibility
- Uses appropriate credentials for Central Portal (note: snapshots don't require signing, so GPG credentials are correctly omitted)
- Publishes to the Central Snapshots repository with
-Prelease=falseThe implementation aligns with the PR's migration to Central Portal publishing.
.github/workflows/release-maven-central.yml (1)
1-38: LGTM! Clear deprecation notice with helpful migration guidance.The deprecated workflow appropriately:
- Retains the file for reference with clear deprecation notice
- Fails immediately with exit code 1 if triggered
- Provides clear instructions for both AUTOMATIC and USER_MANAGED publishing options
- Points users to the replacement workflow and Central Portal
This is a good pattern for deprecating workflows while guiding users to the new approach.
build.gradle.kts (2)
19-28: LGTM! Central Portal configuration is correct.The nmcpAggregation configuration properly:
- Sources credentials from environment variables (secure)
- Allows publishingType to be configured via Gradle property with sensible AUTOMATIC default
- Uses the aggregation plugin's API to publish all maven-publish projects
The method name
publishAllProjectsProbablyBreakingProjectIsolation()is explicit about the project isolation trade-off, which is a known characteristic of aggregation-based publishing.
30-49: LGTM! Clear and comprehensive release documentation.The updated release procedure documentation:
- References the correct new workflows for releases and snapshots
- Provides manual publishing commands for local testing
- Lists all required environment variables with clear naming
- Properly documents the migration to Central Portal API
This documentation will help maintainers understand the new release process.
There was a problem hiding this comment.
Pull request overview
This PR modernizes TestNG's release infrastructure by migrating from the legacy OSSRH (Sonatype Nexus) system to the new Maven Central Portal API, addressing issue #3254. The changes significantly simplify the release process while adding comprehensive documentation for contributors and maintainers.
Key Changes:
- Replaced the old
stage-vote-releaseplugin with thecom.gradleup.nmcp.aggregationplugin for Maven Central Portal publishing - Added extensive documentation covering release procedures, build system architecture, CI workflows, and Java version requirements
- Updated GitHub Actions workflows to support both AUTOMATIC and USER_MANAGED publishing modes
- Deprecated the old two-step release workflow in favor of a streamlined single-step process
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/RELEASE_PROCESS.md | Comprehensive guide for publishing releases to Maven Central Portal with both automatic and manual workflows |
| docs/README.md | Documentation index and quick start guide for building and developing TestNG |
| docs/JAVA_VERSIONS_QUICK_REFERENCE.md | Explains the three Java versions used in TestNG's build (runtime, build toolchain, target bytecode) |
| docs/CI_TEST_WORKFLOW.md | Documents the matrix-based CI testing strategy across multiple Java versions, OSes, and configurations |
| docs/BUILD_SYSTEM.md | Technical documentation of Gradle build system, toolchains, and publishing configuration |
| build.gradle.kts | Integrates nmcp plugin for Central Portal publishing, removes old release infrastructure |
| build-logic/publishing/testng.signing.gradle.kts | Updates signing configuration to support environment variables for CI/CD |
| build-logic/publishing/testng.maven-publish.gradle.kts | Configures snapshot repository for Central Portal |
| build-logic/build-parameters/build.gradle.kts | Adds new build parameters for release control and publishing types |
| .github/workflows/publish-maven-central.yml | Updated workflow for publishing releases via Central Portal |
| .github/workflows/publish-snapshot.yml | Automated snapshot publishing on master branch pushes |
| .github/workflows/release-maven-central.yml | Deprecated with notice explaining new process |
| README.md | Adds Java 11+ runtime requirement and JDK 17+ build requirement |
| .github/CONTRIBUTING.md | Documents JDK 17 requirement for building TestNG |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
build-logic/publishing/src/main/kotlin/testng.maven-publish.gradle.kts
Outdated
Show resolved
Hide resolved
|
@krmahadevan Thanks for the update. I will need more time to understand what is done. Did you succeed to deploy a snapshot version? Could you check if IA reviews are legit? |
I have not tested this. Lets say in this case I just |
|
@juherr - I have addressed all the review comments. I have also tested this locally and it works fine. I have now included a local testing guide as well so that it lists all the steps that followed |
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (3)
docs/RELEASE_PROCESS.md (3)
257-282: Verify the snapshot repository URL is correct.The documentation states snapshots are published to
https://s01.oss.sonatype.org/content/repositories/snapshots/. Since this PR migrates from the legacy OSSRH to the new Central Portal API, verify that:
- The Central Portal still uses this URL for snapshots (or if there's a new endpoint)
- The snapshots workflow (.github/workflows/publish-snapshot.yml) is configured with the correct repository URL
What is the correct Maven Central snapshot repository URL for the new Central Portal API?
96-99: Critical: Secret names are inconsistent with build script.The documentation references
NEXUS_USERNAMEandNEXUS_PASSWORD, but the build.gradle.kts file (lines 24-25) usesCENTRAL_PORTAL_USERNAMEandCENTRAL_PORTAL_PASSWORD. This mismatch will cause authentication failures during publishing.See the comment on build.gradle.kts lines 22-30 for the recommended fix. Ensure these variable names are aligned across the codebase and GitHub Actions workflows.
117-117: Fix formatting: Missing period.The line should end with a period for consistency with other numbered steps.
🔎 Proposed fix
-5.Click **"Run workflow"**. +5. Click **"Run workflow"**.Note: Also add a space after the number for consistency.
🧹 Nitpick comments (2)
docs/BUILD_SYSTEM.md (1)
26-38: Consider adding language specifiers to fenced code blocks and wrapping bare URL.For better markdown compliance:
- Add language specifiers to fenced code blocks (lines 26, 95, 149, 202) - use
textfor ASCII diagrams- Wrap the bare URL at line 264 in angle brackets:
<https://central.sonatype.com/publishing>Also applies to: 95-112, 149-163, 202-235, 264-264
docs/RELEASE_PROCESS.md (1)
1-685: Excellent comprehensive documentation!This documentation is thorough and well-structured, covering:
- ✅ Clear step-by-step workflows for both automatic and manual releases
- ✅ Detailed troubleshooting section
- ✅ Visual diagrams showing the release flow
- ✅ Comparison with the old process
- ✅ Quick reference for common commands
- ✅ Post-release activities checklist
The documentation will significantly help maintainers understand and execute the new release process. Once the environment variable naming issues and command verification are resolved, this will be a valuable addition.
Optional improvements (can be deferred):
The static analysis flagged several style issues that could improve readability:
- Add language identifiers to fenced code blocks (e.g.,
```bash,```text)- Consider wrapping bare URLs in angle brackets or proper markdown links for some URL references
These are purely cosmetic and don't affect functionality.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
.github/CONTRIBUTING.md.github/workflows/publish-maven-central.yml.github/workflows/publish-snapshot.yml.github/workflows/release-maven-central.yml.github/workflows/test.ymlbuild-logic/build-parameters/build.gradle.ktsbuild-logic/publishing/build.gradle.ktsbuild-logic/publishing/src/main/kotlin/testng.maven-publish.gradle.ktsbuild-logic/publishing/src/main/kotlin/testng.signing.gradle.ktsbuild.gradle.ktsdocs/BUILD_SYSTEM.mddocs/JAVA_VERSIONS_QUICK_REFERENCE.mddocs/LOCAL_TESTING_GUIDE.mddocs/RELEASE_PROCESS.md
💤 Files with no reviewable changes (1)
- .github/workflows/release-maven-central.yml
✅ Files skipped from review due to trivial changes (1)
- docs/LOCAL_TESTING_GUIDE.md
🚧 Files skipped from review as they are similar to previous changes (2)
- build-logic/publishing/src/main/kotlin/testng.maven-publish.gradle.kts
- .github/CONTRIBUTING.md
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-09-30T05:21:41.566Z
Learnt from: krmahadevan
Repo: testng-team/testng PR: 3181
File: testng-core/src/test/java/test/invocationcount/FailedInvocationCountTest.java:162-162
Timestamp: 2024-09-30T05:21:41.566Z
Learning: TestNG requires a minimum Java version of JDK 11.
Applied to files:
docs/JAVA_VERSIONS_QUICK_REFERENCE.md
🧬 Code graph analysis (1)
build-logic/publishing/build.gradle.kts (1)
build-logic/publishing/src/main/kotlin/buildlogic/OptionalFeaturesExtension.kt (1)
api(28-30)
🪛 LanguageTool
docs/BUILD_SYSTEM.md
[uncategorized] ~270-~270: The official name of this software platform is spelled with a capital “H”.
Context: ...ion and publishing. ### Test Workflow (.github/workflows/test.yml) The test workflow...
(GITHUB)
[uncategorized] ~276-~276: The official name of this software platform is spelled with a capital “H”.
Context: ...nfiguration The matrix is generated by .github/workflows/matrix.js: - **Java version...
(GITHUB)
[uncategorized] ~315-~315: The official name of this software platform is spelled with a capital “H”.
Context: ...trix version ``` ### Publish Workflow (.github/workflows/publish-maven-central.yml) ...
(GITHUB)
[uncategorized] ~332-~332: The official name of this software platform is spelled with a capital “H”.
Context: ...ing_type }} ``` ### Snapshot Workflow (.github/workflows/publish-snapshot.yml) Autom...
(GITHUB)
docs/JAVA_VERSIONS_QUICK_REFERENCE.md
[style] ~192-~192: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ... Do I need to install Java 21 if I only want to run TestNG in my project? A: No! T...
(REP_WANT_TO_VB)
docs/RELEASE_PROCESS.md
[style] ~165-~165: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...or release and want extra caution - You want to coordinate the release timing #### Ste...
(REP_WANT_TO_VB)
[style] ~339-~339: Consider using “email”.
Context: ...e"** ### 3. Send Release Announcement Send an email to the TestNG users mailing list: To:...
(SEND_AN_EMAIL)
[style] ~412-~412: Consider an alternative verb to strengthen your wording.
Context: ...he workflow already uses JDK 17. If you see this error, check: - The workflow file...
(IF_YOU_HAVE_THIS_PROBLEM)
[uncategorized] ~656-~656: The official name of this software platform is spelled with a capital “H”.
Context: ...-----| | Publish to Maven Central | .github/workflows/publish-maven-central.yml | ...
(GITHUB)
[uncategorized] ~657-~657: The official name of this software platform is spelled with a capital “H”.
Context: ...e publishing | | Publish Snapshot | .github/workflows/publish-snapshot.yml | Autom...
(GITHUB)
[uncategorized] ~658-~658: The official name of this software platform is spelled with a capital “H”.
Context: ...ishing on push to master | | Test | .github/workflows/test.yml | Run tests on PRs ...
(GITHUB)
🪛 markdownlint-cli2 (0.18.1)
docs/BUILD_SYSTEM.md
26-26: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
95-95: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
149-149: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
202-202: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
264-264: Bare URL used
(MD034, no-bare-urls)
docs/JAVA_VERSIONS_QUICK_REFERENCE.md
15-15: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
217-217: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
docs/RELEASE_PROCESS.md
27-27: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
96-96: Bare URL used
(MD034, no-bare-urls)
97-97: Bare URL used
(MD034, no-bare-urls)
109-109: Bare URL used
(MD034, no-bare-urls)
139-139: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
148-148: Bare URL used
(MD034, no-bare-urls)
149-149: Bare URL used
(MD034, no-bare-urls)
169-169: Bare URL used
(MD034, no-bare-urls)
197-197: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
202-202: Bare URL used
(MD034, no-bare-urls)
230-230: Bare URL used
(MD034, no-bare-urls)
231-231: Bare URL used
(MD034, no-bare-urls)
325-325: Bare URL used
(MD034, no-bare-urls)
342-342: Bare URL used
(MD034, no-bare-urls)
348-348: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
428-428: Bare URL used
(MD034, no-bare-urls)
431-431: Bare URL used
(MD034, no-bare-urls)
452-452: Bare URL used
(MD034, no-bare-urls)
463-463: Bare URL used
(MD034, no-bare-urls)
464-464: Bare URL used
(MD034, no-bare-urls)
495-495: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
509-509: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
531-531: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
569-569: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
644-644: Bare URL used
(MD034, no-bare-urls)
645-645: Bare URL used
(MD034, no-bare-urls)
646-646: Bare URL used
(MD034, no-bare-urls)
647-647: Bare URL used
(MD034, no-bare-urls)
648-648: Bare URL used
(MD034, no-bare-urls)
649-649: Bare URL used
(MD034, no-bare-urls)
650-650: Bare URL used
(MD034, no-bare-urls)
677-677: Bare URL used
(MD034, no-bare-urls)
678-678: Bare URL used
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: 25, microsoft, same hashcode, macos, America/New_York, de_DE, stress JIT
- GitHub Check: 26, oracle, ubuntu, UTC, tr_TR
- GitHub Check: 25, oracle, windows, America/New_York, ru_RU, stress JIT
- GitHub Check: 11, microsoft, macos, Pacific/Chatham, tr_TR, stress JIT
- GitHub Check: 21, microsoft, windows, America/New_York, tr_TR, stress JIT
- GitHub Check: 17, temurin, macos, America/New_York, ru_RU, stress JIT
- GitHub Check: 17, liberica, macos, UTC, tr_TR, stress JIT
🔇 Additional comments (18)
.github/workflows/test.yml (1)
56-62: LGTM! Java 21 update aligns with nmcp plugin requirements.The workflow correctly installs both the matrix test version and Java 21, with Java 21 as the default for running Gradle. This change is consistent with the nmcp plugin requirement (Java 17+) and the broader migration to Java 21 as the standard build toolchain.
build-logic/publishing/src/main/kotlin/testng.signing.gradle.kts (2)
16-22: LGTM! Good credential sourcing pattern for CI/CD.The configuration correctly supports both project properties and environment variables, with proper null checking before calling
useInMemoryPgpKeys. This is a solid pattern for local development and CI/CD environments.
24-38: LGTM! Signing configuration properly addresses past review concern.The configuration correctly:
- Gates signing based on
signing.pgp.enabledandreleaseparameters- Calls
sign()on all publications inafterEvaluate(addressing the past review comment about missing sign() call)- Uses proper Gradle providers API for lazy evaluation
The
afterEvaluateis necessary because publications may not exist at configuration time.docs/BUILD_SYSTEM.md (2)
40-57: LGTM! Clear explanation of Java version requirements.The documentation accurately explains why Java 21 is required for all Gradle commands, including the crucial insight that the nmcp plugin is loaded during configuration phase for every build command. This helps users understand why they can't use Java 11 to run Gradle.
1-474: Excellent comprehensive documentation for the build system.This documentation significantly improves the project's maintainability by clearly explaining:
- The three-layer Java version model (Gradle runtime, build toolchain, target bytecode, test runtime)
- Build parameters and their usage
- Publishing workflows and architecture
- CI/CD configuration
- Troubleshooting guidance
The content is well-organized, technically accurate, and provides valuable context for developers and maintainers.
build-logic/build-parameters/build.gradle.kts (2)
21-23: LGTM! Java 21 default aligns with nmcp requirements.The default change from 17 to 21 is correct for the nmcp plugin compatibility.
Note: The description mentions "JMeter" instead of "TestNG" - this appears to be a copy-paste error from another project. Similar issues exist on lines 26, 29, 32, 35, 38. Consider fixing these references in a follow-up.
56-75: LGTM! Well-defined parameters for release and publishing control.The three new parameters are properly configured:
release- Controls snapshot vs release versions (safe default: false)centralPortal.publishingType- Controls automatic vs manual publishing (default: AUTOMATIC)signing.pgp.enabled- Controls signing behavior (default: AUTO for releases)All parameters have clear descriptions and sensible defaults.
docs/JAVA_VERSIONS_QUICK_REFERENCE.md (2)
1-108: LGTM! Clear and accurate explanation of Java version layers.The documentation effectively explains the three-layer Java version model:
- Gradle Runtime (Java 21) - for nmcp plugin
- Build Toolchain (Java 21) - for modern compilation
- Target Bytecode (Java 11) - for compatibility via --release flag
- Test Runtime (Configurable) - for multi-version testing
This aligns with the retrieved learning that TestNG requires minimum Java 11 and the build parameter defaults (jdkBuildVersion=21, targetJavaVersion=11).
Based on learnings: TestNG requires minimum Java version of JDK 11.
152-195: LGTM! FAQ effectively addresses common Java version questions.The FAQ section clearly answers key questions:
- Why Java 21 is required for Gradle (nmcp plugin requirement)
- How --release flag ensures Java 11 compatibility
- How Gradle toolchains enable multi-version testing
- Distinction between building TestNG vs using TestNG
These answers will help developers avoid common configuration mistakes.
.github/workflows/publish-maven-central.yml (3)
5-16: LGTM! Well-designed workflow input for publishing control.The
publishing_typeinput correctly:
- Uses
choicetype to prevent invalid values- Offers AUTOMATIC and USER_MANAGED options matching the build parameter enum
- Defaults to USER_MANAGED (safer option requiring manual approval)
32-53: Verify that NEXUS_ secrets contain Central Portal credentials.*The workflow maps
secrets.NEXUS_USERNAMEandsecrets.NEXUS_PASSWORDtoCENTRAL_PORTAL_USERNAMEandCENTRAL_PORTAL_PASSWORD. Since this PR migrates from the old OSSRH system to the new Central Portal API, please verify that these secrets contain the correct credentials for the Central Portal (not legacy OSSRH credentials).According to the PR objectives, the old OSSRH system is being sunset, and the new Central Portal requires different credentials obtained from https://central.sonatype.com/.
55-63: LGTM! Clear completion messages for both publishing types.The conditional messages appropriately guide users based on the selected publishing type, providing relevant URLs for checking status (AUTOMATIC) or manually approving the release (USER_MANAGED).
Important: Per the PR comments, this workflow has NOT been tested yet. Please test the publishing workflow (preferably with a snapshot or test release) before merging to ensure the NMCP integration works correctly.
.github/workflows/publish-snapshot.yml (2)
18-36: Verify Central Portal credentials and test the snapshot publishing workflow.The workflow configuration looks correct, but please verify:
Credentials: Confirm that
secrets.NEXUS_USERNAMEandsecrets.NEXUS_PASSWORDcontain valid Central Portal credentials (not legacy OSSRH credentials).Testing: This workflow runs automatically on every push to master. Per your PR comment, snapshot deployment has NOT been tested yet. It's critical to test this workflow before merging, as any failure will break automatic snapshot publishing.
Recommend testing by:
- Temporarily enabling the workflow on your feature branch, or
- Manually running the Gradle command locally with Central Portal credentials
38-41: LGTM! Clear completion message with artifact URL.The completion message provides appropriate feedback and includes the Central Portal artifact URL where users can verify the published snapshot.
build-logic/publishing/build.gradle.kts (1)
8-8: No issues found with the nmcp plugin version.Version 1.4.1 was released on Dec 30, 2025, and is a valid, intentional upgrade from the previously referenced 1.4.0.
build.gradle.kts (2)
8-11: LGTM! Version calculation logic is clear.The version calculation correctly uses the
releaseproperty to determine whether to append-SNAPSHOTsuffix. This aligns with the new publishing workflow.
29-29: The methodpublishAllProjectsProbablyBreakingProjectIsolation()breaks Gradle project isolation and should be reconsidered.This is a convenience method from the nmcp plugin that uses
allprojects {}block traversal, which is explicitly incompatible with Gradle's Project Isolation feature. This can cause configuration cache issues and prevent parallelizable builds.If Project Isolation is a concern, use these alternatives instead:
- Apply the
com.gradleup.nmcpplugin directly to each module you want to publish, then list those modules in the root aggregation config- Explicitly configure
publishAllPublicationsToCentralPortalor individually list publications instead of using the shortcut methoddocs/RELEASE_PROCESS.md (1)
289-300: Task name is correct and properly configured.The
publishAllPublicationsToCentralSnapshotsRepositorytask is correctly derived from the Maven Publish plugin, not the nmcp plugin. The repository is defined inbuild-logic/publishing/src/main/kotlin/testng.maven-publish.gradle.ktswith name"centralSnapshots", and Maven Publish auto-generates the task name in the formatpublish<PublicationName>To<RepositoryName>Repository. The comparison withpublishAggregationToCentralPortalis not applicable—that is an nmcp plugin task for releases, while this task handles snapshot publishing via Maven Publish.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/test.ymlbuild-logic-commons/gradle-plugin/build.gradle.ktsbuild-logic-commons/gradle-plugin/src/main/kotlin/build-logic.kotlin-dsl-gradle-plugin.gradle.ktsbuild-logic/jvm/build.gradle.ktsbuild-logic/jvm/src/main/kotlin/testng.kotlin-library.gradle.ktstestng-core/testng-core-build.gradle.kts
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/test.yml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-09-30T05:21:41.566Z
Learnt from: krmahadevan
Repo: testng-team/testng PR: 3181
File: testng-core/src/test/java/test/invocationcount/FailedInvocationCountTest.java:162-162
Timestamp: 2024-09-30T05:21:41.566Z
Learning: TestNG requires a minimum Java version of JDK 11.
Applied to files:
testng-core/testng-core-build.gradle.kts
🧬 Code graph analysis (1)
build-logic/jvm/build.gradle.kts (1)
build-logic/publishing/src/main/kotlin/buildlogic/OptionalFeaturesExtension.kt (1)
api(28-30)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: 25, liberica, macos, America/New_York, fr_FR
🔇 Additional comments (5)
build-logic/jvm/src/main/kotlin/testng.kotlin-library.gradle.kts (2)
9-9: LGTM: Kotlin BOM version aligns with Gradle plugin.The Kotlin BOM version 2.3.0 correctly aligns with the Kotlin Gradle plugin version in
build-logic/jvm/build.gradle.kts.
14-19: LGTM: Migration to compilerOptions API is complete and correct.The migration from deprecated
kotlinOptionstocompilerOptionsis properly implemented:
- Correct use of
freeCompilerArgs.add()instead of+=operator- Proper usage of
jvmTarget.set()withJvmTarget.fromTarget()for Kotlin 2.x API- No remaining
kotlinOptionsusages exist in the codebase—the migration is complete across the build logic.build-logic/jvm/build.gradle.kts (1)
10-10: No issues found. Kotlin 2.3.0 is a stable general release (December 16, 2025), and the codebase is already compatible with it. The Gradle plugin version (2.3.0) aligns with the Kotlin BOM version (2.3.0), and the requiredcompilerOptionsmigration is already in place in the codebase.build-logic-commons/gradle-plugin/src/main/kotlin/build-logic.kotlin-dsl-gradle-plugin.gradle.kts (1)
12-13: The code change [21, 11] is correct and intentional; no modification needed.This change is part of the documented upgrade to Java 21 as the minimum build requirement. The project now requires Java 21 to run Gradle and compile, as confirmed in LOCAL_TESTING_GUIDE.md, BUILD_SYSTEM.md, and JAVA_VERSIONS_QUICK_REFERENCE.md. The fallback to Java 11 bytecode is appropriate since it's the target compatibility version for artifacts.
However, CONTRIBUTING.md is outdated and should be updated from "JDK 17 or higher" to "JDK 21 or higher" and change
jdkBuildVersion=17tojdkBuildVersion=21to reflect the current requirements. This documentation update would clarify that Java 17-20 is no longer supported for building (only for testing via Gradle toolchains).The suggestion to use
[21, 17, 11]is incorrect because Java 17 is no longer a valid build target after this migration.Likely an incorrect or invalid review comment.
testng-core/testng-core-build.gradle.kts (1)
64-65: Good documentation explaining the classpath management change.The comments clearly explain why manual Kotlin classpath configuration was removed. This is helpful for future maintainers. The Kotlin version is confirmed to be 2.3.0 in the build configuration, validating the accuracy of the comment.
krmahadevan
force-pushed
the
task/fix_3254
branch
from
1f35bc6 to
29dbcec
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
docs/BUILD_SYSTEM.md (1)
202-218: Update nmcp plugin version in documentation to match implementation.Line 207 shows version "1.0.3", but according to the previous review comment, the actual implementation uses version "1.4.1" (in build-logic/publishing/build.gradle.kts). Please update the documentation to reflect the correct version to ensure users following this guide use the same version as the project.
🔎 Proposed fix
│ plugins { │ -│ id("com.gradleup.nmcp.aggregation") version "1.0.3" │ +│ id("com.gradleup.nmcp.aggregation") version "1.4.1" │ │ } │
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.github/workflows/test.ymlbuild-logic-commons/gradle-plugin/build.gradle.ktsbuild-logic-commons/gradle-plugin/src/main/kotlin/build-logic.kotlin-dsl-gradle-plugin.gradle.ktsbuild-logic/jvm/build.gradle.ktsbuild-logic/jvm/src/main/kotlin/testng.kotlin-library.gradle.ktsdocs/BUILD_SYSTEM.mddocs/RELEASE_PROCESS.mdtestng-core/testng-core-build.gradle.kts
🚧 Files skipped from review as they are similar to previous changes (3)
- build-logic-commons/gradle-plugin/build.gradle.kts
- build-logic-commons/gradle-plugin/src/main/kotlin/build-logic.kotlin-dsl-gradle-plugin.gradle.kts
- build-logic/jvm/build.gradle.kts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-09-30T05:21:41.566Z
Learnt from: krmahadevan
Repo: testng-team/testng PR: 3181
File: testng-core/src/test/java/test/invocationcount/FailedInvocationCountTest.java:162-162
Timestamp: 2024-09-30T05:21:41.566Z
Learning: TestNG requires a minimum Java version of JDK 11.
Applied to files:
testng-core/testng-core-build.gradle.kts
🧬 Code graph analysis (1)
build-logic/jvm/src/main/kotlin/testng.kotlin-library.gradle.kts (1)
build-logic/publishing/src/main/kotlin/buildlogic/OptionalFeaturesExtension.kt (1)
platform(25-26)
🪛 LanguageTool
docs/BUILD_SYSTEM.md
[uncategorized] ~270-~270: The official name of this software platform is spelled with a capital “H”.
Context: ...ion and publishing. ### Test Workflow (.github/workflows/test.yml) The test workflow...
(GITHUB)
[uncategorized] ~276-~276: The official name of this software platform is spelled with a capital “H”.
Context: ...nfiguration The matrix is generated by .github/workflows/matrix.js: - **Java version...
(GITHUB)
[uncategorized] ~315-~315: The official name of this software platform is spelled with a capital “H”.
Context: ...trix version ``` ### Publish Workflow (.github/workflows/publish-maven-central.yml) ...
(GITHUB)
[uncategorized] ~332-~332: The official name of this software platform is spelled with a capital “H”.
Context: ...ing_type }} ``` ### Snapshot Workflow (.github/workflows/publish-snapshot.yml) Autom...
(GITHUB)
docs/RELEASE_PROCESS.md
[style] ~167-~167: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...or release and want extra caution - You want to coordinate the release timing #### Ste...
(REP_WANT_TO_VB)
[style] ~343-~343: Consider using “email”.
Context: ...e"** ### 3. Send Release Announcement Send an email to the TestNG users mailing list: To:...
(SEND_AN_EMAIL)
[style] ~416-~416: Consider an alternative verb to strengthen your wording.
Context: ...he workflow already uses JDK 21. If you see this error, check: - The workflow file...
(IF_YOU_HAVE_THIS_PROBLEM)
[uncategorized] ~670-~670: The official name of this software platform is spelled with a capital “H”.
Context: ...-----| | Publish to Maven Central | .github/workflows/publish-maven-central.yml | ...
(GITHUB)
[uncategorized] ~671-~671: The official name of this software platform is spelled with a capital “H”.
Context: ...e publishing | | Publish Snapshot | .github/workflows/publish-snapshot.yml | Autom...
(GITHUB)
[uncategorized] ~672-~672: The official name of this software platform is spelled with a capital “H”.
Context: ...ishing on push to master | | Test | .github/workflows/test.yml | Run tests on PRs ...
(GITHUB)
🪛 markdownlint-cli2 (0.18.1)
docs/BUILD_SYSTEM.md
27-27: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
96-96: Bare URL used
(MD034, no-bare-urls)
97-97: Bare URL used
(MD034, no-bare-urls)
111-111: Bare URL used
(MD034, no-bare-urls)
141-141: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
150-150: Bare URL used
(MD034, no-bare-urls)
151-151: Bare URL used
(MD034, no-bare-urls)
171-171: Bare URL used
(MD034, no-bare-urls)
199-199: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
204-204: Bare URL used
(MD034, no-bare-urls)
232-232: Bare URL used
(MD034, no-bare-urls)
233-233: Bare URL used
(MD034, no-bare-urls)
329-329: Bare URL used
(MD034, no-bare-urls)
346-346: Bare URL used
(MD034, no-bare-urls)
352-352: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
432-432: Bare URL used
(MD034, no-bare-urls)
435-435: Bare URL used
(MD034, no-bare-urls)
456-456: Bare URL used
(MD034, no-bare-urls)
467-467: Bare URL used
(MD034, no-bare-urls)
468-468: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (10)
testng-core/testng-core-build.gradle.kts (2)
44-48: LGTM! Groovy upgrade correctly addresses Java 21 bytecode compatibility.The upgrade to Groovy 4.0.29 is correct and aligns with the PR's Java 21 migration objectives. Version 4.0.29 is the latest stable 4.x release and properly supports Java 21 bytecode (class file major version 65), which Groovy 3.x cannot read. The explanatory comments clearly document the rationale, and this change addresses the previous review comment recommending the upgrade to 4.0.29.
64-65: The project is confirmed to be using Kotlin 2.3.0, which validates the note. The Kotlin JVM plugin version is explicitly set to 2.3.0 inbuild-logic/jvm/build.gradle.kts, and the comment accurately documents that theclasspathproperty on theKotlinCompiletask has been removed in this version. No manual Kotlin classpath configuration is needed or present.build-logic/jvm/src/main/kotlin/testng.kotlin-library.gradle.kts (2)
14-19: LGTM! Correct migration to modern Kotlin Gradle DSL.The migration from
kotlinOptionstocompilerOptionsfollows the recommended approach for modern Kotlin Gradle plugin versions. The changes correctly:
- Use
freeCompilerArgs.add()instead of the+=operator- Apply type-safe
JvmTarget.fromTarget()instead of string-basedjvmTargetassignment- Maintain the existing
-Xjvm-default=alland-Xjdk-releaseconfigurationThis modernization aligns with the Kotlin 2.3.0 upgrade and improves type safety.
9-9: Ensure full compatibility testing across the codebase for this major Kotlin upgrade.Kotlin 2.3.0 is a valid release, but upgrading from 1.6.20 introduces significant breaking changes requiring comprehensive verification:
- Language version support: Kotlin 2.3 removes support for
-language-version=1.8on all platforms. Verify all source files target a supported language version (2.x).- Gradle API: The migration from
kotlinOptionstocompilerOptionsshown in this file is correct; ensure this pattern is applied consistently across all build scripts.- Deprecated APIs: stdlib APIs (locale-sensitive case conversions, etc.) and context receivers have been removed or replaced. Scan the codebase for deprecated API usage.
- Android Gradle Plugin: If using AGP, verify compatibility (minimum 8.2.2). The
kotlin-androidplugin is deprecated for AGP ≥ 9.0.0.- Incremental testing recommended: Consider upgrading through intermediate versions (1.7 → 1.8 → 2.0 → 2.1 → 2.2 → 2.3) if immediate full jump encounters issues, using Kotlin's migration tooling at each step.
Confirm that breaking changes have been addressed across the full codebase, not just this build script.
.github/workflows/test.yml (2)
56-64: LGTM! Java 21 setup aligns with build system requirements.The step name and inline comments clearly explain that Java 21 is installed and becomes the default (being last in the list), while the matrix Java version is used for running tests. This aligns with the documented requirement in BUILD_SYSTEM.md that the nmcp plugin requires Java 17+ and the decision to use Java 21 as the latest LTS for running Gradle and building TestNG.
88-88: LGTM! Build version parameter updated consistently.The
jdkBuildVersion=21parameter correctly reflects the new default Java version for building, consistent with the Java 21 installation configured earlier in the workflow.docs/BUILD_SYSTEM.md (2)
40-57: Excellent explanation of Java version requirements!The table and explanation clearly communicate the multi-version Java strategy and why Java 21 is required for all builds (not just publishing). The note that the nmcp plugin is loaded during Gradle's configuration phase for every build command is particularly helpful for developers who might otherwise think they only need Java 21 for publishing tasks.
1-474: Comprehensive and well-structured build system documentation.This documentation provides excellent coverage of TestNG's build system, including:
- Clear explanation of the composite build structure
- Detailed Java version requirements and toolchain configuration
- Practical examples of common build commands
- Helpful troubleshooting guidance
- Visual diagrams illustrating key concepts
The addition of this documentation will significantly help developers understand and work with TestNG's build system.
docs/RELEASE_PROCESS.md (2)
80-101: Excellent documentation of secret mapping!The table clearly explains the mapping between GitHub secret names (e.g.,
NEXUS_USERNAME) and the environment variables required by the build (e.g.,CENTRAL_PORTAL_USERNAME). The note at line 101 is particularly helpful in clarifying that manual publishing requires using the environment variable names, not the secret names. This will prevent a common source of confusion.
1-699: Comprehensive release process documentation!This documentation provides excellent coverage of the release process including:
- Clear step-by-step instructions for both automatic and manual releases
- Visual workflow diagrams that make the process easy to understand
- Detailed troubleshooting guidance for common issues
- Helpful comparison with the old OSSRH process showing the improvements
- Quick reference section with all necessary commands and URLs
The addition of this documentation will significantly help maintainers execute releases confidently and consistently.
krmahadevan
force-pushed
the
task/fix_3254
branch
from
29dbcec to
714b9f9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
docs/RELEASE_PROCESS.md (2)
119-119: Fix formatting: add space after period.Line 119 still has the formatting inconsistency from the previous review: "5.Click" should be "5. Click" to match spacing in other numbered steps.
🔎 Proposed fix
-5.Click **"Run workflow"**. +5. Click **"Run workflow"**.
288-297: Fix task name in manual snapshot publishing command.Line 293 still uses the incorrect task
publishAllPublicationsToCentralSnapshotsRepository. This was flagged in the previous review but remains unfixed. The Quick Reference section (line 641) correctly usespublishAggregationToCentralPortalSnapshots—update the manual section to match for consistency.🔎 Proposed fix
### Manual Snapshot Publishing If you need to manually publish a snapshot: ```bash -./gradlew publishAllPublicationsToCentralSnapshotsRepository \ +./gradlew publishAggregationToCentralPortalSnapshots \ -Prelease=false \ --no-daemon \ --stacktrace</details> </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (9)</summary><blockquote> <details> <summary>docs/RELEASE_PROCESS.md (6)</summary><blockquote> `27-27`: **Add language specifier to code block.** The code block on line 27 lacks a language specifier. While this is purely a Markdown formatting issue, adding one improves syntax highlighting and readability. Add ` ```text` or similar to the opening fence. --- `141-141`: **Add language specifier to code block.** The code block on line 141 lacks a language specifier. While this is purely a Markdown formatting issue, adding one improves syntax highlighting and readability. --- `199-199`: **Add language specifier to code block.** The code block on line 199 lacks a language specifier. While this is purely a Markdown formatting issue, adding one improves syntax highlighting and readability. --- `354-354`: **Add language specifier to code block.** The code block on line 354 lacks a language specifier. While this is purely a Markdown formatting issue, adding one improves syntax highlighting and readability. --- `501-501`: **Add language specifiers to code blocks.** Lines 501, 515, 537, and 575 contain code blocks without language specifiers. While this is purely a Markdown formatting issue, adding them improves syntax highlighting and readability. Consider specifying appropriate languages (e.g., `text` for process diagrams, `bash` for shell commands). Also applies to: 515-515, 537-537, 575-575 --- `672-674`: **Capitalize "GitHub" in table entries.** Lines 672–674 have "**Publish to Maven Central**", "**Publish Snapshot**", and "**Test**" in the workflow table, but LanguageTool flags that the official spelling is "GitHub" (with capital H) when referring to the platform. While this appears in workflow descriptions rather than the name itself, ensure consistency with GitHub's brand guidelines. </blockquote></details> <details> <summary>docs/BUILD_SYSTEM.md (3)</summary><blockquote> `270-270`: **Capitalize GitHub correctly in workflow section headings.** The platform name is "GitHub" (capital G and H). Lines 270, 276, 315, and 332 should read "GitHub Workflows" rather than "Github Workflows" in the context descriptions. <details> <summary>🔎 Proposed fixes</summary> ```diff ### Test Workflow (`.github/workflows/test.yml`) - The test workflow uses a matrix strategy to test across multiple Java versions, operating systems, and configurations. + The GitHub test workflow uses a matrix strategy to test across multiple Java versions, operating systems, and configurations. #### Matrix Configuration - The matrix is generated by `.github/workflows/matrix.js`: + The matrix is generated by the GitHub Actions configuration in `.github/workflows/matrix.js`: ### Publish Workflow (`.github/workflows/publish-maven-central.yml`) - Manually triggered workflow for publishing releases: + Manually triggered GitHub workflow for publishing releases: ### Snapshot Workflow (`.github/workflows/publish-snapshot.yml`) - Automatically publishes snapshots on every push to master: + Automatically publishes snapshots to GitHub on every push to master:Also applies to: 276-276, 315-315, 332-332
26-38: Add language specifications to all fenced code blocks.Fenced code blocks should specify a language identifier for consistency and proper rendering. The ASCII diagrams at lines 26, 95, 149, and 202 should be marked as plain text or converted to indented code blocks.
🔎 Proposed fixes
### Project Structure - ``` + ```text testng/ ├── build-logic/ # Composite build for build logic ... - ``` + ```text ┌────────────────────────────────────────────────────────┐ │ Gradle Process (runs with Java 21) │ ... - ``` + ```text testng.java-library (most projects use this) ├── testng.java ... - ``` + ```text ┌───────────────────────────────────────────────-──────────┐ │ Root Project (build.gradle.kts) │ ...Also applies to: 95-112, 149-163, 202-218
466-472: Ensure URLs in documentation are properly formatted as markdown links.Review the resources section to confirm all URLs follow markdown link syntax
[text](url)rather than appearing as bare URLs. The current section appears well-formatted, but this is a best practice for maintainability.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/CONTRIBUTING.md.github/workflows/test.ymlbuild-logic-commons/gradle-plugin/build.gradle.ktsbuild-logic-commons/gradle-plugin/src/main/kotlin/build-logic.kotlin-dsl-gradle-plugin.gradle.ktsbuild-logic/jvm/build.gradle.ktsbuild-logic/jvm/src/main/kotlin/testng.kotlin-library.gradle.ktsbuild-logic/publishing/src/main/kotlin/testng.maven-publish.gradle.ktsdocs/BUILD_SYSTEM.mddocs/RELEASE_PROCESS.mdtestng-core/testng-core-build.gradle.kts
🚧 Files skipped from review as they are similar to previous changes (7)
- testng-core/testng-core-build.gradle.kts
- .github/workflows/test.yml
- .github/CONTRIBUTING.md
- build-logic-commons/gradle-plugin/src/main/kotlin/build-logic.kotlin-dsl-gradle-plugin.gradle.kts
- build-logic/jvm/src/main/kotlin/testng.kotlin-library.gradle.kts
- build-logic/publishing/src/main/kotlin/testng.maven-publish.gradle.kts
- build-logic/jvm/build.gradle.kts
🧰 Additional context used
🪛 LanguageTool
docs/RELEASE_PROCESS.md
[style] ~167-~167: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...or release and want extra caution - You want to coordinate the release timing #### Ste...
(REP_WANT_TO_VB)
[style] ~345-~345: Consider using “email”.
Context: ...e"** ### 3. Send Release Announcement Send an email to the TestNG users mailing list: To:...
(SEND_AN_EMAIL)
[style] ~418-~418: Consider an alternative verb to strengthen your wording.
Context: ...he workflow already uses JDK 21. If you see this error, check: - The workflow file...
(IF_YOU_HAVE_THIS_PROBLEM)
[uncategorized] ~672-~672: The official name of this software platform is spelled with a capital “H”.
Context: ...-----| | Publish to Maven Central | .github/workflows/publish-maven-central.yml | ...
(GITHUB)
[uncategorized] ~673-~673: The official name of this software platform is spelled with a capital “H”.
Context: ...e publishing | | Publish Snapshot | .github/workflows/publish-snapshot.yml | Autom...
(GITHUB)
[uncategorized] ~674-~674: The official name of this software platform is spelled with a capital “H”.
Context: ...ishing on push to master | | Test | .github/workflows/test.yml | Run tests on PRs ...
(GITHUB)
docs/BUILD_SYSTEM.md
[uncategorized] ~270-~270: The official name of this software platform is spelled with a capital “H”.
Context: ...ion and publishing. ### Test Workflow (.github/workflows/test.yml) The test workflow...
(GITHUB)
[uncategorized] ~276-~276: The official name of this software platform is spelled with a capital “H”.
Context: ...nfiguration The matrix is generated by .github/workflows/matrix.js: - **Java version...
(GITHUB)
[uncategorized] ~315-~315: The official name of this software platform is spelled with a capital “H”.
Context: ...trix version ``` ### Publish Workflow (.github/workflows/publish-maven-central.yml) ...
(GITHUB)
[uncategorized] ~332-~332: The official name of this software platform is spelled with a capital “H”.
Context: ...ing_type }} ``` ### Snapshot Workflow (.github/workflows/publish-snapshot.yml) Autom...
(GITHUB)
🪛 markdownlint-cli2 (0.18.1)
docs/RELEASE_PROCESS.md
27-27: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
96-96: Bare URL used
(MD034, no-bare-urls)
97-97: Bare URL used
(MD034, no-bare-urls)
111-111: Bare URL used
(MD034, no-bare-urls)
141-141: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
150-150: Bare URL used
(MD034, no-bare-urls)
151-151: Bare URL used
(MD034, no-bare-urls)
171-171: Bare URL used
(MD034, no-bare-urls)
199-199: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
204-204: Bare URL used
(MD034, no-bare-urls)
232-232: Bare URL used
(MD034, no-bare-urls)
233-233: Bare URL used
(MD034, no-bare-urls)
331-331: Bare URL used
(MD034, no-bare-urls)
348-348: Bare URL used
(MD034, no-bare-urls)
354-354: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
434-434: Bare URL used
(MD034, no-bare-urls)
437-437: Bare URL used
(MD034, no-bare-urls)
458-458: Bare URL used
(MD034, no-bare-urls)
469-469: Bare URL used
(MD034, no-bare-urls)
470-470: Bare URL used
(MD034, no-bare-urls)
501-501: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
515-515: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
537-537: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
575-575: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
660-660: Bare URL used
(MD034, no-bare-urls)
661-661: Bare URL used
(MD034, no-bare-urls)
662-662: Bare URL used
(MD034, no-bare-urls)
663-663: Bare URL used
(MD034, no-bare-urls)
664-664: Bare URL used
(MD034, no-bare-urls)
665-665: Bare URL used
(MD034, no-bare-urls)
666-666: Bare URL used
(MD034, no-bare-urls)
693-693: Bare URL used
(MD034, no-bare-urls)
694-694: Bare URL used
(MD034, no-bare-urls)
docs/BUILD_SYSTEM.md
26-26: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
95-95: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
149-149: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
202-202: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
264-264: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (2)
docs/BUILD_SYSTEM.md (1)
1-472: Overall documentation is comprehensive and well-structured.The BUILD_SYSTEM.md provides excellent coverage of the Gradle build system, Java toolchain configuration, publishing architecture, and CI/CD workflows. The explanations are clear, diagrams aid understanding, and practical examples support the concepts. Documentation aligns well with the PR's goals of migrating to nmcp-based Central Portal publishing with Java 21 as the build platform.
Pending verification of the nmcp plugin version accuracy, this documentation is ready.
build-logic-commons/gradle-plugin/build.gradle.kts (1)
18-19: Java 21 toolchain selection logic looks good.The updated logic correctly prefers Java 21 and gracefully falls back to Java 11 when running on older JVMs. The comment accurately describes the behavior. The Gradle wrapper (8.11.1) and kotlin-dsl plugin with dynamic versioning fully support Java 21.
| │ Root Project (build.gradle.kts) │ | ||
| │ │ | ||
| │ plugins { │ | ||
| │ id("com.gradleup.nmcp.aggregation") version "1.0.3" │ |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n -A2 -B2 'com\.gradleup\.nmcp' --type gradleRepository: testng-team/testng
Length of output: 210
🏁 Script executed:
#!/bin/bash
# Find gradle files and search for nmcp version
fd -e "gradle.kts" -e "gradle" | head -20
echo "---"
# Search for nmcp without type restrictions
rg "com\.gradleup\.nmcp" -A2 -B2Repository: testng-team/testng
Length of output: 4066
Update nmcp plugin version to match actual codebase.
Line 207 documents version "1.0.3", but the actual codebase uses version "1.4.1" (as specified in build.gradle.kts and build-logic/publishing/build.gradle.kts). Update the documentation to reflect the current version.
🤖 Prompt for AI Agents
In docs/BUILD_SYSTEM.md around line 207, the documented plugin version "1.0.3"
is out of date; update that line to show the current nmcp plugin version "1.4.1"
to match build.gradle.kts and build-logic/publishing/build.gradle.kts so the
docs reflect the codebase.
2 participants
Fixes #3254
Fixing the release process.
Also added documentation to help fix the release process
Fixes #3254 .
Summary by CodeRabbit
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.