Fix Release process#3255
Conversation
|
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. 📝 WalkthroughWalkthroughTestNG migrates its release infrastructure from deprecated OSSRH to Maven Central Portal API using the Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
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.
|
@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.
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.
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.
|
@juherr - Did you get time to vet this out? Do you think can we try merging this PR and then checking if it works in terms of publishing snapshots and releases ? |
juherr
left a comment
There was a problem hiding this comment.
LGTM. Please just add a header on the top of the docs. Something like this:
Audience:
- Contributors
- Maintainers
- Release managers
- ...
When to read this:
- First read / Reference / Troubleshooting
- ...
TL;DR:
- aaa
- bbbb
- ...
-----
and check if Java 17 is "removed" everywhere.
|
|
||
| ### Prerequisites | ||
|
|
||
| - **Java 17+** installed (required to run Gradle) |
| 1. **Gradle Runtime**: Java 17+ (required by nmcp plugin) | ||
| 2. **Build Toolchain**: Java 17 (for compilation) | ||
| 3. **Target Bytecode**: Java 11 (for compatibility) | ||
| 4. **Test Runtime**: Configurable (11, 17, 21, 24) |
There was a problem hiding this comment.
| 1. **Gradle Runtime**: Java 17+ (required by nmcp plugin) | |
| 2. **Build Toolchain**: Java 17 (for compilation) | |
| 3. **Target Bytecode**: Java 11 (for compatibility) | |
| 4. **Test Runtime**: Configurable (11, 17, 21, 24) | |
| 1. **Gradle Runtime**: Java 21+ | |
| 2. **Build Toolchain**: Java 21 (for compilation) | |
| 3. **Target Bytecode**: Java 11 (for compatibility) | |
| 4. **Test Runtime**: Configurable (11, 17, 21, 24) |
| 3. **Target Bytecode**: Java 11 (for compatibility) | ||
| 4. **Test Runtime**: Configurable (11, 17, 21, 24) | ||
|
|
||
| **Result:** TestNG artifacts work on Java 11+, but you need Java 17+ to build. |
There was a problem hiding this comment.
| **Result:** TestNG artifacts work on Java 11+, but you need Java 17+ to build. | |
| **Result:** TestNG artifacts work on Java 11+, but you need Java 21+ to build. |
|
|
||
| ```bash | ||
| # Java versions | ||
| -PjdkBuildVersion=17 # JDK for building (default: 17) |
There was a problem hiding this comment.
| -PjdkBuildVersion=17 # JDK for building (default: 17) | |
| -PjdkBuildVersion=21 # JDK for building (default: 21) |
| │ Java Version Usage in TestNG Build │ | ||
| ├──────────────────────────────────────────────────────────────┤ | ||
| │ │ | ||
| │ Gradle Runtime: Java 21 (latest LTS) │ |
There was a problem hiding this comment.
| │ Gradle Runtime: Java 21 (latest LTS) │ | |
| │ Gradle Runtime: Java 21 (LTS before latest LTS) │ |
| 1. **Install Java 17+** | ||
| ```bash | ||
| # Using SDKMAN | ||
| sdk install java 17.0.14-tem | ||
| sdk use java 17.0.14-tem |
There was a problem hiding this comment.
| 1. **Install Java 17+** | |
| ```bash | |
| # Using SDKMAN | |
| sdk install java 17.0.14-tem | |
| sdk use java 17.0.14-tem | |
| 1. **Install Java 21+** | |
| ```bash | |
| # Using SDKMAN | |
| sdk install java 21.y.z-tem | |
| sdk use java 21.y.z-tem |
|
|
||
| ### Common Issues | ||
|
|
||
| 1. **"Nmcp requires Java 17+"** |
|
@juherr - I have fixed all the documentation discrepancies that you had mentioned. Please check now. My apologies for the delayed response. Been a bit held up at the personal front. |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@docs/BUILD_SYSTEM.md`:
- Around line 26-38: Several fenced code blocks showing the ASCII tree (starting
with "testng/") are missing a language identifier which triggers markdownlint
MD040; update each unlabeled triple-backtick fence that wraps the diagram (the
blocks containing the "testng/" tree) to include an explicit language tag (use
```text) so the diagram blocks are labeled consistently across the file.
- Around line 263-265: In the USER_MANAGED description line replace the bare URL
with a Markdown link (e.g., replace "https://central.sonatype.com/publishing"
with "[Sonatype publishing portal](https://central.sonatype.com/publishing)") so
the USER_MANAGED bullet uses a proper Markdown link and satisfies markdownlint
MD034; update the line that starts with "- **USER_MANAGED**:" accordingly.
In `@docs/JAVA_VERSIONS_QUICK_REFERENCE.md`:
- Around line 15-39: The markdown diagram code fences in
docs/JAVA_VERSIONS_QUICK_REFERENCE.md are unlabeled, causing markdownlint MD040;
update both ASCII diagram code blocks (the triple-backtick fenced diagrams
showing "Your Machine" / "Gradle Process") to use a language tag by replacing
``` with ```text so each fence begins with ```text and ends with ```, ensuring
both diagram blocks in the file are updated.
In `@docs/README.md`:
- Around line 144-156: The fenced code block showing the directory tree
beginning with "testng/" is missing a language label; open the block with
```text (i.e., change the opening triple backticks to ```text) to satisfy
markdownlint MD040 and leave the block content unchanged; update the fenced
block that starts with "testng/" in the README.md.
♻️ Duplicate comments (3)
docs/README.md (1)
114-118: Align test runtime list with matrix (25/26 instead of 24)
The matrix and other docs now include Java 25 and 26; the list here still mentions 24.🧩 Suggested fix
-4. **Test Runtime**: Configurable (11, 17, 21, 24) +4. **Test Runtime**: Configurable (11, 17, 21, 25, 26)docs/RELEASE_PROCESS.md (2)
629-646: Critical: Quick reference contains incorrect snapshot task name.Line 641 uses
publishAllPublicationsToCentralSnapshotsRepository, which is incorrect. The correct nmcp plugin task ispublishAggregationToCentralPortalSnapshots. This was flagged in a previous review (marked as "Addressed in commit 714b9f9"), but the fix was not applied or was reverted.This is especially problematic in the Quick Reference section because users will copy-paste these commands directly.
🔧 Corrected command block
# Publish release (automatic) ./gradlew publishAggregationToCentralPortal \ -Prelease=true \ -PcentralPortal.publishingType=AUTOMATIC # Publish release (manual) ./gradlew publishAggregationToCentralPortal \ -Prelease=true \ -PcentralPortal.publishingType=USER_MANAGED # Publish snapshot -./gradlew publishAllPublicationsToCentralSnapshotsRepository \ +./gradlew publishAggregationToCentralPortalSnapshots \ -Prelease=false # Build without publishing ./gradlew build -Prelease=true
288-297: Critical: Incorrect snapshot publishing task name persists despite past fix.Line 293 still uses
publishAllPublicationsToCentralSnapshotsRepository, which is wrong. According to the nmcp plugin API and the previous review (marked as "Addressed in commit 714b9f9"), the correct task ispublishAggregationToCentralPortalSnapshots. This fix appears to have been reverted or not properly applied.Users following this documentation will get a task-not-found error or unexpected behavior.
🔧 Corrected command
-./gradlew publishAllPublicationsToCentralSnapshotsRepository \ +./gradlew publishAggregationToCentralPortalSnapshots \ -Prelease=false \ --no-daemon \ --stacktrace
🧹 Nitpick comments (2)
testng-core/src/test/java/test/retryAnalyzer/issue3231/MutationSample.java (1)
43-55: Consider makingmaxRetryCountfinal.The field is never modified after initialization, making it effectively a constant.
Suggested change
public static class MyRetry implements IRetryAnalyzer { private int retryCount = 0; - private int maxRetryCount = 3; + private final int maxRetryCount = 3; `@Override` public boolean retry(ITestResult result) {testng-runner-api/src/main/java/org/testng/internal/TestResult.java (1)
367-369: No callers of this method exist in the codebase.Search found only the method definition itself—no calls to
setParameterIndexanywhere in the repository. This indicates the team has already cleaned up all usages before converting it to a no-op, which is an intentional pattern before removal. The@Deprecated(forRemoval = true)annotation clearly signals removal intent to external users.While throwing
UnsupportedOperationExceptioncould be more explicit for late-binding reflection calls, the current approach is functionally safe since no internal callers are affected.
| ``` | ||
| testng/ | ||
| ├── build-logic/ # Composite build for build logic | ||
| │ ├── build-parameters/ # Defines build parameters | ||
| │ ├── basics/ # Basic utilities and toolchain config | ||
| │ ├── jvm/ # Java/Kotlin compilation plugins | ||
| │ ├── code-quality/ # Testing and quality plugins | ||
| │ └── publishing/ # Maven publishing and signing | ||
| ├── testng-core/ # Core TestNG implementation | ||
| ├── testng-api/ # Public API | ||
| ├── testng/ # Main distribution (merged jar) | ||
| └── [other modules]/ | ||
| ``` |
There was a problem hiding this comment.
Add language identifiers to fenced blocks (markdownlint MD040)
Several fenced diagrams are missing a language tag (e.g., Line 26). Adding text avoids lint failures and improves editor rendering.
🧩 Suggested fix (apply to each unlabeled fence)
-```
+```text
testng/
├── build-logic/ # Composite build for build logic
│ ├── build-parameters/ # Defines build parameters
│ ├── basics/ # Basic utilities and toolchain config
│ ├── jvm/ # Java/Kotlin compilation plugins
│ ├── code-quality/ # Testing and quality plugins
│ └── publishing/ # Maven publishing and signing
├── testng-core/ # Core TestNG implementation
├── testng-api/ # Public API
├── testng/ # Main distribution (merged jar)
└── [other modules]/
-```
+```Also applies to: 95-112, 149-163, 202-235
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
26-26: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In `@docs/BUILD_SYSTEM.md` around lines 26 - 38, Several fenced code blocks
showing the ASCII tree (starting with "testng/") are missing a language
identifier which triggers markdownlint MD040; update each unlabeled
triple-backtick fence that wraps the diagram (the blocks containing the
"testng/" tree) to include an explicit language tag (use ```text) so the diagram
blocks are labeled consistently across the file.
| - **AUTOMATIC**: Artifacts are automatically published to Maven Central after validation | ||
| - **USER_MANAGED**: Artifacts are staged in the portal; you manually publish from https://central.sonatype.com/publishing | ||
|
|
There was a problem hiding this comment.
Avoid bare URL in USER_MANAGED description (markdownlint MD034)
Wrap the portal URL in a Markdown link to satisfy linting.
🧩 Suggested fix
-- **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)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - **AUTOMATIC**: Artifacts are automatically published to Maven Central after validation | |
| - **USER_MANAGED**: Artifacts are staged in the portal; you manually publish from https://central.sonatype.com/publishing | |
| - **AUTOMATIC**: Artifacts are automatically published to Maven Central after validation | |
| - **USER_MANAGED**: Artifacts are staged in the portal; you manually publish from [Central Portal](https://central.sonatype.com/publishing) | |
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
264-264: Bare URL used
(MD034, no-bare-urls)
🤖 Prompt for AI Agents
In `@docs/BUILD_SYSTEM.md` around lines 263 - 265, In the USER_MANAGED description
line replace the bare URL with a Markdown link (e.g., replace
"https://central.sonatype.com/publishing" with "[Sonatype publishing
portal](https://central.sonatype.com/publishing)") so the USER_MANAGED bullet
uses a proper Markdown link and satisfies markdownlint MD034; update the line
that starts with "- **USER_MANAGED**:" accordingly.
| ``` | ||
| ┌─────────────────────────────────────────────────────────┐ | ||
| │ Your Machine │ | ||
| │ │ | ||
| │ Java 21 (default) ──────────────┐ │ | ||
| │ Java 11 (installed) │ │ | ||
| │ │ │ | ||
| │ ┌───────────────────────────────▼─────────────────-─┐ │ | ||
| │ │ Gradle Process (runs with Java 21) │ │ | ||
| │ │ │ │ | ||
| │ │ ┌───────────────────────────────────────────--─┐ │ │ | ||
| │ │ │ Compile Task │ │ │ | ||
| │ │ │ • Uses: Java 21 toolchain │ │ │ | ||
| │ │ │ • Produces: Java 11 bytecode │ │ │ | ||
| │ │ └──────────────────────────────────────────--──┘ │ │ | ||
| │ │ │ │ | ||
| │ │ ┌──────────────────────────────────────────-─-─┐ │ │ | ||
| │ │ │ Test Task │ │ │ | ||
| │ │ │ • Spawns new JVM with: Java 11 │ │ │ | ||
| │ │ │ • Runs tests in that JVM │ │ │ | ||
| │ │ └──────────────────────────────────────────--──┘ │ │ | ||
| │ │ │ │ | ||
| │ └───────────────────────────────────────────────-───┘ │ | ||
| └─────────────────────────────────────────────────────────┘ | ||
| ``` |
There was a problem hiding this comment.
Label diagram fences with text (markdownlint MD040)
Adding a language tag avoids lint errors and improves renderer behavior.
🧩 Suggested fix (apply to both diagram blocks)
-```
+```text
┌─────────────────────────────────────────────────────────┐
│ Your Machine │
│ │
│ Java 21 (default) ──────────────┐ │
│ Java 11 (installed) │ │
│ │ │
│ ┌───────────────────────────────▼─────────────────-─┐ │
│ │ Gradle Process (runs with Java 21) │ │
│ │ │ │
│ │ ┌───────────────────────────────────────────--─┐ │ │
│ │ │ Compile Task │ │ │
│ │ │ • Uses: Java 21 toolchain │ │ │
│ │ │ • Produces: Java 11 bytecode │ │ │
│ │ └──────────────────────────────────────────--──┘ │ │
│ │ │ │
│ │ ┌──────────────────────────────────────────-─-─┐ │ │
│ │ │ Test Task │ │ │
│ │ │ • Spawns new JVM with: Java 11 │ │ │
│ │ │ • Runs tests in that JVM │ │ │
│ │ └──────────────────────────────────────────--──┘ │ │
│ │ │ │
│ └───────────────────────────────────────────────-───┘ │
└─────────────────────────────────────────────────────────┘
-```
+```Also applies to: 217-230
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
15-15: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In `@docs/JAVA_VERSIONS_QUICK_REFERENCE.md` around lines 15 - 39, The markdown
diagram code fences in docs/JAVA_VERSIONS_QUICK_REFERENCE.md are unlabeled,
causing markdownlint MD040; update both ASCII diagram code blocks (the
triple-backtick fenced diagrams showing "Your Machine" / "Gradle Process") to
use a language tag by replacing ``` with ```text so each fence begins with
```text and ends with ```, ensuring both diagram blocks in the file are updated.
| ``` | ||
| testng/ | ||
| ├── build-logic/ # Composite build for build logic | ||
| │ ├── build-parameters/ # Build parameter definitions | ||
| │ ├── basics/ # Toolchain utilities | ||
| │ ├── jvm/ # Java/Kotlin plugins | ||
| │ ├── code-quality/ # Testing plugins | ||
| │ └── publishing/ # Maven publishing | ||
| ├── testng-core/ # Core implementation | ||
| ├── testng-api/ # Public API | ||
| ├── testng/ # Main distribution | ||
| └── docs/ # This documentation | ||
| ``` |
There was a problem hiding this comment.
Add language to project-structure fence (markdownlint MD040)
Label the directory tree block as text to avoid lint issues.
🧩 Suggested fix
-```
+```text
testng/
├── build-logic/ # Composite build for build logic
│ ├── build-parameters/ # Build parameter definitions
│ ├── basics/ # Toolchain utilities
│ ├── jvm/ # Java/Kotlin plugins
│ ├── code-quality/ # Testing plugins
│ └── publishing/ # Maven publishing
├── testng-core/ # Core implementation
├── testng-api/ # Public API
├── testng/ # Main distribution
└── docs/ # This documentation
-```
+```🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
144-144: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In `@docs/README.md` around lines 144 - 156, The fenced code block showing the
directory tree beginning with "testng/" is missing a language label; open the
block with ```text (i.e., change the opening triple backticks to ```text) to
satisfy markdownlint MD040 and leave the block content unchanged; update the
fenced block that starts with "testng/" in the README.md.
Fixes testng-team#3254 Fixing the release process. Also added documentation to help fix the release process
a0c0762 to
7a7f607
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @.github/workflows/publish-snapshot.yml:
- Around line 28-36: Replace the legacy Gradle task
publishAllPublicationsToCentralSnapshotsRepository with the nmcp aggregate task
publishAggregationToCentralPortalSnapshots in the workflow step and add the
required GPG signing env vars SIGNING_PGP_PRIVATE_KEY and SIGNING_PGP_PASSPHRASE
(plus reuse existing CENTRAL_PORTAL_USERNAME and CENTRAL_PORTAL_PASSWORD) so the
snapshot publish runs with nmcp and signs artifacts; update the run command to
invoke publishAggregationToCentralPortalSnapshots and export the signing env
vars in the step environment block to mirror the release workflow's signing
setup.
In `@docs/CI_TEST_WORKFLOW.md`:
- Around line 35-72: The markdown file contains unlabeled fenced code blocks
(markdownlint MD040); add appropriate language tags to each fenced block (e.g.,
```text for the ASCII diagrams, ```json for the example matrix object, ```yaml
or ```bash where applicable) so linters can detect them; specifically update the
ASCII diagram block that begins with the box characters (the GitHub Actions
Workflow: test.yml diagram) to use ```text and label the JSON example inside the
same block as ```json if you split it, and apply the same labeling convention to
all other unlabeled fenced blocks in docs/CI_TEST_WORKFLOW.md.
In `@docs/RELEASE_PROCESS.md`:
- Line 699: Update the "Last Updated" year string in the document: replace the
literal "**Last Updated**: 2024" with "**Last Updated**: 2025" so the release
process header reflects the 2025 Central Portal migration (look for the exact
text "**Last Updated**: 2024" in RELEASE_PROCESS.md and change the year).
- Around line 253-256: Replace the incorrect statement "no signing required for
snapshots" in the release steps with a clear requirement that all artifacts
(including snapshots) must be signed with GPG/PGP and include corresponding .asc
signature files, i.e., change the bullet that currently reads "no signing
required for snapshots" to state that GPG/PGP signatures are required for all
deployed files (including snapshots); also update the "Last Updated" date
currently set to 2024 to 2025 so the document header matches the noted 2025
OSSRH changes.
♻️ Duplicate comments (3)
docs/BUILD_SYSTEM.md (2)
26-38: Add language tags to fenced diagrams (markdownlint MD040).
Several code fences are unlabeled; add a language tag (e.g.,text) and apply this to all unlabeled fences in the document.🧩 Proposed fix (apply similarly to other unlabeled fences)
-``` +```text testng/ ├── build-logic/ # Composite build for build logic │ ├── build-parameters/ # Defines build parameters │ ├── basics/ # Basic utilities and toolchain config │ ├── jvm/ # Java/Kotlin compilation plugins │ ├── code-quality/ # Testing and quality plugins │ └── publishing/ # Maven publishing and signing ├── testng-core/ # Core TestNG implementation ├── testng-api/ # Public API ├── testng/ # Main distribution (merged jar) └── [other modules]/</details> --- `263-265`: **Replace bare URL in USER_MANAGED bullet (markdownlint MD034).** Use a Markdown link instead of a raw URL. <details> <summary>🧩 Proposed fix</summary> ```diff -- **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/JAVA_VERSIONS_QUICK_REFERENCE.md (1)
15-39: Label ASCII diagram fences with a language tag (markdownlint MD040).
Please addtextto the diagram fences here and in the visual summary block.🧩 Proposed fix (apply to both diagram fences)
-``` +```text ┌─────────────────────────────────────────────────────────┐ │ Your Machine │ │ │ │ Java 21 (default) ──────────────┐ │ │ Java 11 (installed) │ │ │ │ │ ┌───────────────────────────────▼─────────────────-─┐ │ │ │ Gradle Process (runs with Java 21) │ │ │ │ │ │ ┌───────────────────────────────────────────--─┐ │ │ │ │ Compile Task │ │ │ │ │ • Uses: Java 21 toolchain │ │ │ │ │ • Produces: Java 11 bytecode │ │ │ │ └──────────────────────────────────────────--──┘ │ │ │ │ │ │ ┌──────────────────────────────────────────-─-─┐ │ │ │ │ Test Task │ │ │ │ │ • Spawns new JVM with: Java 11 │ │ │ │ │ • Runs tests in that JVM │ │ │ │ └──────────────────────────────────────────--──┘ │ │ │ │ │ └───────────────────────────────────────────────-───┘ └─────────────────────────────────────────────────────────┘</details> Also applies to: 217-230 </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (2)</summary><blockquote> <details> <summary>.github/workflows/publish-snapshot.yml (1)</summary><blockquote> `38-41`: **Minor: Snapshot URL points to release artifact page.** The URL `https://central.sonatype.com/artifact/org.testng/testng` shows release artifacts, not snapshots. Consider updating to point to the snapshots repository browser or clarify that users should check the snapshot repository URL documented elsewhere. <details> <summary>💡 Suggested improvement</summary> ```diff - name: Display completion message run: | echo "✅ Snapshot published to Maven Central Snapshots repository" - echo "Snapshots are available at: https://central.sonatype.com/artifact/org.testng/testng" + echo "Snapshots are available at: https://central.sonatype.com/repository/maven-snapshots/" + echo "Add snapshot repository to consume: https://central.sonatype.com/repository/maven-snapshots/"docs/README.md (1)
144-156: Add language specifier to fenced code block (markdownlint MD040).The project structure diagram is missing a language specifier. Add
textto satisfy the linter.📝 Suggested fix
-``` +```text testng/ ├── build-logic/ # Composite build for build logic │ ├── build-parameters/ # Build parameter definitions │ ├── basics/ # Toolchain utilities │ ├── jvm/ # Java/Kotlin plugins │ ├── code-quality/ # Testing plugins │ └── publishing/ # Maven publishing ├── testng-core/ # Core implementation ├── testng-api/ # Public API ├── testng/ # Main distribution └── docs/ # This documentation</details> </blockquote></details> </blockquote></details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| ``` | ||
| ┌─────────────────────────────────────────────────────────────┐ | ||
| │ GitHub Actions Workflow: test.yml │ | ||
| └─────────────────────────────────────────────────────────────┘ | ||
| │ | ||
| ▼ | ||
| ┌─────────────────────────────────────────────────────────────┐ | ||
| │ Job 1: matrix_prep │ | ||
| │ │ | ||
| │ Runs: matrix.js │ | ||
| │ Produces: JSON matrix with 7 job configurations │ | ||
| │ │ | ||
| │ Example output: │ | ||
| │ { │ | ||
| │ "include": [ │ | ||
| │ { │ | ||
| │ "java_version": "<LTS_VERSION>", │ | ||
| │ "java_distribution": "temurin", │ | ||
| │ "os": "ubuntu-latest", │ | ||
| │ "tz": "UTC", │ | ||
| │ "locale": {"language": "en", "country": "US"}, │ | ||
| │ ... │ | ||
| │ }, │ | ||
| │ ... │ | ||
| │ ] │ | ||
| │ } │ | ||
| └─────────────────────────────────────────────────────────────┘ | ||
| │ | ||
| ▼ | ||
| ┌─────────────────────────────────────────────────────────────┐ | ||
| │ Job 2: build (runs in parallel for each matrix entry) │ | ||
| │ │ | ||
| │ For each matrix configuration: │ | ||
| │ 1. Install Java versions (matrix version + build version) │ | ||
| │ 2. Run Gradle build with specific parameters │ | ||
| │ 3. Upload test reports if failed │ | ||
| └─────────────────────────────────────────────────────────────┘ | ||
| ``` |
There was a problem hiding this comment.
Add language tags to fenced blocks (markdownlint MD040).
Several fenced blocks are unlabeled; please add appropriate language tags (e.g., text, yaml, json, bash) throughout the file.
🧩 Example fix for the first diagram block
-```
+```text
┌─────────────────────────────────────────────────────────────┐
│ GitHub Actions Workflow: test.yml │
└─────────────────────────────────────────────────────────────┘
│
▼
┌─────────────────────────────────────────────────────────────┐
│ Job 1: matrix_prep │
│ │
│ Runs: matrix.js │
│ Produces: JSON matrix with 7 job configurations │
│ │
│ Example output: │
│ { │
│ "include": [ │
│ { │
│ "java_version": "<LTS_VERSION>", │
│ "java_distribution": "temurin", │
│ "os": "ubuntu-latest", │
│ "tz": "UTC", │
│ "locale": {"language": "en", "country": "US"}, │
│ ... │
│ }, │
│ ... │
│ ] │
│ } │
└─────────────────────────────────────────────────────────────┘
│
▼
┌─────────────────────────────────────────────────────────────┐
│ Job 2: build (runs in parallel for each matrix entry) │
│ │
│ For each matrix configuration: │
│ 1. Install Java versions (matrix version + build version) │
│ 2. Run Gradle build with specific parameters │
│ 3. Upload test reports if failed │
└─────────────────────────────────────────────────────────────┘</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
In @docs/CI_TEST_WORKFLOW.md around lines 35 - 72, The markdown file contains
unlabeled fenced code blocks (markdownlint MD040); add appropriate language tags
to each fenced block (e.g., text for the ASCII diagrams, json for the
example matrix object, yaml or bash where applicable) so linters can
detect them; specifically update the ASCII diagram block that begins with the
box characters (the GitHub Actions Workflow: test.yml diagram) to use text and label the JSON example inside the same block as json if you split it, and
apply the same labeling convention to all other unlabeled fenced blocks in
docs/CI_TEST_WORKFLOW.md.
</details>
<!-- fingerprinting:phantom:poseidon:eagle -->
<!-- This is an auto-generated comment by CodeRabbit -->
| 1. ✅ Build artifacts (no signing required for snapshots) | ||
| 2. ✅ Upload to Central Snapshots repository | ||
| 3. ✅ Immediately available | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Maven Central Repository snapshot publishing signing requirements 2025
💡 Result:
Short answer — as of Jan 21, 2026:
- GPG/PGP signatures are required for files uploaded to the Central Publisher Portal: every deployed file must have a corresponding .asc signature. [1]
- Snapshot (-SNAPSHOT) publishing is supported via the Central Publisher Portal (announced Jan 14, 2025). Snapshots are handled by a dedicated snapshot repository (https://central.sonatype.com/repository/maven-snapshots/) and you must enable SNAPSHOTs for your namespace in the Portal before publishing. Validation behavior for SNAPSHOTs differs from release components; see the portal docs for the exact workflow. [2][3]
- The portal now also accepts and validates Sigstore signatures (introduced Jan 29, 2025), but PGP signing remains required. Sigstore is optional today but invalid Sigstore signatures will produce warnings (and may become blocking later). [4]
If you want, I can list the exact files you must sign and a minimal Maven/Gradle snippet to publish snapshots to the portal.
Sources: [1] Sonatype Central requirements, [2] Sonatype announcement (Jan 14, 2025), [3] Sonatype snapshot publishing docs, [4] Sonatype blog on Sigstore validation.
🏁 Script executed:
# Find and read the RELEASE_PROCESS.md file to get context
fd -type f -name "RELEASE_PROCESS.md" | head -5Repository: testng-team/testng
Length of output: 232
🏁 Script executed:
# Search for RELEASE_PROCESS.md
find . -name "RELEASE_PROCESS.md" -type fRepository: testng-team/testng
Length of output: 87
🏁 Script executed:
# Once found, read the relevant sections
cat -n docs/RELEASE_PROCESS.md | sed -n '240,270p'Repository: testng-team/testng
Length of output: 1019
🏁 Script executed:
# Also check for "last updated" date reference
grep -n "last updated\|Last updated\|2024\|2025" docs/RELEASE_PROCESS.md | tail -10Repository: testng-team/testng
Length of output: 422
Correct the snapshot signing requirement on line 253.
Line 253 incorrectly states "no signing required for snapshots". As of January 2025, Maven Central Portal requires GPG/PGP signatures for all deployed files, including snapshots. Every file must have a corresponding .asc signature file. Update this line to reflect the actual signing requirement.
Additionally, the "Last Updated" date on line 699 should be changed from 2024 to 2025 to align with the documented events (e.g., OSSRH shutdown on June 30, 2025 mentioned on line 286).
🤖 Prompt for AI Agents
In `@docs/RELEASE_PROCESS.md` around lines 253 - 256, Replace the incorrect
statement "no signing required for snapshots" in the release steps with a clear
requirement that all artifacts (including snapshots) must be signed with GPG/PGP
and include corresponding .asc signature files, i.e., change the bullet that
currently reads "no signing required for snapshots" to state that GPG/PGP
signatures are required for all deployed files (including snapshots); also
update the "Last Updated" date currently set to 2024 to 2025 so the document
header matches the noted 2025 OSSRH changes.
|
|
||
| --- | ||
|
|
||
| **Last Updated**: 2024 (Updated for Maven Central Portal migration) |
There was a problem hiding this comment.
Fix the "Last Updated" year.
The document states "Last Updated: 2024" but this PR is for Central Portal migration happening in 2025 (per the PR context mentioning OSSRH EOL 2025-06-30). Update to reflect the correct year.
📝 Suggested fix
-**Last Updated**: 2024 (Updated for Maven Central Portal migration)
+**Last Updated**: 2025 (Updated for Maven Central Portal migration)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| **Last Updated**: 2024 (Updated for Maven Central Portal migration) | |
| **Last Updated**: 2025 (Updated for Maven Central Portal migration) |
🤖 Prompt for AI Agents
In `@docs/RELEASE_PROCESS.md` at line 699, Update the "Last Updated" year string
in the document: replace the literal "**Last Updated**: 2024" with "**Last
Updated**: 2025" so the release process header reflects the 2025 Central Portal
migration (look for the exact text "**Last Updated**: 2024" in
RELEASE_PROCESS.md and change the year).
juherr
left a comment
There was a problem hiding this comment.
It looks great. I didn't check if all AI comments are legit.
Feel free to merge once you think you have fixed everything needed.
Fixes #3254
Fixing the release process.
Also added documentation to help fix the release process
Fixes #3254 .
Summary by CodeRabbit
Release Notes
Build Requirements
Publishing & Release Process
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.