Skip to content

[PM Fixer] Add pnpm 12.x case to TestPnpmVersionValidation - #553

Closed
agrasth wants to merge 1 commit into
mainfrom
RTECO-1037-pnpm12-version-test
Closed

[PM Fixer] Add pnpm 12.x case to TestPnpmVersionValidation#553
agrasth wants to merge 1 commit into
mainfrom
RTECO-1037-pnpm12-version-test

Conversation

@agrasth

@agrasth agrasth commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

Summary

TestPnpmVersionValidation already asserts pnpm 10.x and 11.x meet minSupportedPnpmVersion's floor, explicitly documenting "no upper bound" for the 11.x case. pnpm 12.x is now the actual latest tested version (jfrog/jfrog-cli#3692 fixed a real pnpm 12 compatibility bug), so this extends the same explicit pattern to 12.x.

Fix

Added a pnpm12 := version.NewVersion("12.2.1") case to TestPnpmVersionValidation, mirroring the existing pnpm10/pnpm11 cases exactly.

Compatibility impact

Test-only change. No production code touched, no exported symbols changed.

Test plan

  • go test ./artifactory/commands/pnpm/... -run TestPnpmVersionValidation -v — passes
  • go test ./artifactory/commands/pnpm/... (full package) — passes
  • gofmt -l, go vet ./..., go build ./... — all clean

Opened by PM Compat Fixer follow-up, prompted by review of jfrog/jfrog-cli#3692.

Summary by CodeRabbit

  • Tests
    • Added coverage confirming that pnpm version 12.2.1 meets the minimum supported version requirement.

TestPnpmVersionValidation already asserts pnpm 10.x and 11.x meet
minSupportedPnpmVersion's floor (documented as having no upper bound).
pnpm 12.x is now the actual latest tested version (jfrog/jfrog-cli#3692
fixed a real pnpm 12 compatibility bug in a test helper), so extend the
same pattern to it explicitly for reviewer clarity, even though it's
implied by the existing no-upper-bound cases.
@agrasth
agrasth requested a review from a team September 8, 2026 05:56
@agrasth agrasth added the pm-compat-fixer Opened automatically by JFrog's PM Compat Fixer agent label Sep 8, 2026
@coderabbitai

coderabbitai Bot commented Sep 8, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Enterprise

Run ID: 173ec997-b0ca-49ee-a34e-c5670bdba48d

📥 Commits

Reviewing files that changed from the base of the PR and between 4c19791 and f91cb4b.

📒 Files selected for processing (1)
  • artifactory/commands/pnpm/pnpm_test.go

Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.


📝 Walkthrough

Walkthrough

The pnpm test suite adds validation coverage for version 12.2.1. The case confirms that the version meets the minimum supported pnpm version without an upper-version restriction.

Changes

pnpm version validation

Layer / File(s) Summary
pnpm 12 validation test
artifactory/commands/pnpm/pnpm_test.go
Adds a test case that accepts pnpm version 12.2.1 without an upper-version restriction.

Estimated code review effort: 1 (Trivial) | ~2 minutes

Merge Risk: ⚪ Minimal · up to f91cb

This test-only change confirms pnpm 12.2.1 satisfies the existing minimum-version rule and is ready to merge.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: adding a pnpm 12.x test case to TestPnpmVersionValidation.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch RTECO-1037-pnpm12-version-test

Comment @coderabbitai help to get the list of available commands.

@agrasth agrasth added improvement Automatically generated release notes safe to test Approve running integration tests on a pull request labels Sep 8, 2026
@github-actions github-actions Bot removed the safe to test Approve running integration tests on a pull request label Sep 8, 2026

@fluxxBot fluxxBot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

not correct

@agrasth

agrasth commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator Author

Closing - this was based on a misread of what was actually needed. The real gap is jfrog-cli's own CI test matrix (hardcoded to pnpm 10/11, never testing 12 on regular PR merges), not this floor-check test, which is redundant with the existing 'no upper bound' pnpm11 case. Fixed properly in jfrog/jfrog-cli#3705.

@agrasth agrasth closed this Sep 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

improvement Automatically generated release notes pm-compat-fixer Opened automatically by JFrog's PM Compat Fixer agent

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants