Refactor error handling and improve test logging for installers#989
Refactor error handling and improve test logging for installers#989chiranjib-swain wants to merge 4 commits intoactions:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR standardizes “version not found” error handling across Java distribution installers by introducing a shared helper on JavaBase, and updates installer/caching tests to use the new message format while reducing noisy core.error output during test runs.
Changes:
- Added
JavaBase.createVersionNotFoundError(...)to generate consistent, context-rich error messages (with optional available-versions truncation). - Refactored multiple distribution installers to throw the standardized error instead of bespoke strings.
- Updated Jest suites to align assertions with the new messages and to consistently mock
core.errorto suppress test log noise.
Reviewed changes
Copilot reviewed 33 out of 34 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/distributions/base-installer.ts | Adds createVersionNotFoundError helper for standardized errors. |
| src/distributions/adopt/installer.ts | Uses the new helper when no satisfied version is found. |
| src/distributions/corretto/installer.ts | Uses the new helper when no satisfied version is found. |
| src/distributions/dragonwell/installer.ts | Uses the new helper when no satisfied version is found. |
| src/distributions/graalvm/installer.ts | Switches to the helper for some “not found” paths and appends a GraalVM URL hint on 404. |
| src/distributions/jetbrains/installer.ts | Uses the new helper when no satisfied version is found. |
| src/distributions/liberica/installer.ts | Uses the new helper when no satisfied version is found. |
| src/distributions/microsoft/installer.ts | Uses the new helper when no satisfied version is found (includes manifest versions). |
| src/distributions/oracle/installer.ts | Uses the new helper when no satisfied version is found. |
| src/distributions/sapmachine/installer.ts | Uses the new helper when no satisfied version is found. |
| src/distributions/semeru/installer.ts | Uses the new helper and adds platform context to the error. |
| src/distributions/temurin/installer.ts | Uses the new helper when no satisfied version is found. |
| src/distributions/zulu/installer.ts | Uses the new helper when no satisfied version is found. |
| dist/setup/index.js | Updates the compiled distribution bundle to reflect source changes. |
| tests/distributors/adopt-installer.test.ts | Mocks core.error to suppress logs during tests. |
| tests/distributors/base-installer.test.ts | Updates version-not-found expectations and adds unit tests for the helper. |
| tests/distributors/corretto-installer.test.ts | Mocks core.error to suppress logs during tests. |
| tests/distributors/dragonwell-installer.test.ts | Updates error expectations and mocks core.error. |
| tests/distributors/graalvm-installer.test.ts | Updates error expectations and removes assertions tied to old core.error logging behavior. |
| tests/distributors/jetbrains-installer.test.ts | Mocks core.error to suppress logs during tests. |
| tests/distributors/liberica-installer.test.ts | Updates error expectations and mocks core.error. |
| tests/distributors/liberica-linux-installer.test.ts | Updates error expectations and mocks core.error. |
| tests/distributors/liberica-windows-installer.test.ts | Updates error expectations and mocks core.error. |
| tests/distributors/local-installer.test.ts | Mocks core.error to suppress logs during tests. |
| tests/distributors/microsoft-installer.test.ts | Mocks core.error to suppress logs during tests. |
| tests/distributors/oracle-installer.test.ts | Mocks core.error to suppress logs during tests. |
| tests/distributors/sapmachine-installer.test.ts | Updates error expectations and mocks core.error. |
| tests/distributors/semeru-installer.test.ts | Mocks core.error to suppress logs during tests. |
| tests/distributors/temurin-installer.test.ts | Mocks core.error to suppress logs during tests. |
| tests/distributors/zulu-installer.test.ts | Updates error expectations and mocks core.error. |
| tests/distributors/zulu-linux-installer.test.ts | Updates error expectations and mocks core.error. |
| tests/distributors/zulu-windows-installer.test.ts | Updates error expectations and mocks core.error. |
| tests/cache.test.ts | Mocks core.error and strengthens cleanup of Jest mocks between tests. |
| tests/cleanup-java.test.ts | Mocks core.error and strengthens cleanup of Jest mocks between tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
jsoref
left a comment
There was a problem hiding this comment.
I'll try to test this to give additional feedback, but based on the tests, this is my first pass.
| (distribution as any).findPackageForDownload('17.0.99') | ||
| ).rejects.toThrow( | ||
| 'Could not find GraalVM for SemVer 17.0.99. Please check if this version is available at https://download.oracle.com/graalvm' | ||
| "Could not find satisfied version for SemVer '17.0.99'" |
There was a problem hiding this comment.
Fwiw, this message is lousy. satisfied isn't compatible with could not find.
You could say could not satisfy or could not find a satisfying, but the text here doesn't work.
| expect(core.error).toHaveBeenCalledWith( | ||
| 'Available versions: 23-ea-20240716, 23-ea-20240709' | ||
| ).rejects.toThrow( | ||
| "No EA build is marked as 'latest' for version range '23-ea'. Available EA versions: 23-ea-20240716, 23-ea-20240709." |
There was a problem hiding this comment.
This is hard to read. If there's a range, you should use some range notification markers, e.g. ['23-ea'... '23-ea']. Otherwise, it looks like the range is ['23'... 'ea'], but I don't think it is given that a version appears to be '23-ea-something'. And note that you probably should quote (or `) the available versions, or remove that quoting from the first part.
| ); | ||
| // Create the standard error with additional hint about checking the download URL | ||
| const error = this.createVersionNotFoundError(range); | ||
| error.message += `\nPlease check if this version is available at ${GRAALVM_DL_BASE}`; |
There was a problem hiding this comment.
This message isn't actionable. If I find that it is available there, what am I supposed to do?
Beforehttps://github.com/check-spelling-sandbox/setup-java/actions/runs/23344583858 Annotations9 errors Afterhttps://github.com/check-spelling-sandbox/setup-java/actions/runs/23345876163 Annotations3 errors For the rest of it, I'm looking for something like: This is roughly the first 3 lines from each audit report (the package, the severity, and the first line of description). You could be fancy and use |
Description:
This PR standardizes the “version not found” error handling across Java distribution installers by introducing a shared helper in JavaBase. It also updates installer and caching tests to align with the new message format and reduces noisy
core.erroroutput during test runs.Related issue:
#977, #978
Check list: