fix fqns and assertj - #3312
Conversation
Summary of ChangesHello @robfrank, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on improving code quality and consistency within the test suite. The changes primarily involve modernizing JUnit test method declarations, simplifying exception handling in tests, and cleaning up fully qualified name usage by introducing appropriate imports. Additionally, a dependency exclusion was added to the Gremlin module to ensure smoother dependency resolution. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request includes some nice cleanups, such as fixing fully qualified names and updating test method names to align with modern JUnit 5 conventions. However, I've noticed a recurring pattern where specific checked exceptions in throws clauses (like IOException, InterruptedException) are replaced with the generic Exception. This is generally considered a bad practice as it makes method contracts less specific and can obscure the actual exceptions that callers should be prepared to handle. It's better to declare the most specific exception types possible. I've added a few specific comments with suggestions to revert this change in some places. Please apply this feedback across all similar changes in the pull request.
|
|
||
| @Test | ||
| void successMessageCreation() throws IOException { | ||
| void successMessageCreation() throws Exception { |
There was a problem hiding this comment.
The method signature was changed from throws IOException to throws Exception. However, the body of this test method does not appear to throw any checked exceptions. It's best practice to remove the throws clause entirely if no checked exceptions are thrown.
| void successMessageCreation() throws Exception { | |
| void successMessageCreation() { |
|
|
||
| @Test | ||
| void successMessageWriteAndToString() throws IOException { | ||
| void successMessageWriteAndToString() throws Exception { |
There was a problem hiding this comment.
Changing the exception type from IOException to the more generic Exception makes the method's contract less specific. It's better to declare the most specific exception type that can be thrown. Since methods in this test call PackStreamWriter methods that throw IOException, the signature should specify throws IOException.
| void successMessageWriteAndToString() throws Exception { | |
| void successMessageWriteAndToString() throws IOException { |
|
|
||
| @Test | ||
| void concurrentSessions() throws InterruptedException { | ||
| void concurrentSessions() throws Exception { |
There was a problem hiding this comment.
The throws clause was changed from InterruptedException to Exception. It's better to be specific about the exceptions a method can throw. Since thread.join() throws InterruptedException, the method signature should declare throws InterruptedException.
| void concurrentSessions() throws Exception { | |
| void concurrentSessions() throws InterruptedException { |
|
|
||
| @Test | ||
| void floatArrayPropertyRoundTrip() throws SQLException, ClassNotFoundException { | ||
| void floatArrayPropertyRoundTrip() throws Exception { |
There was a problem hiding this comment.
Broadening the exception type from SQLException, ClassNotFoundException to Exception makes the method signature less precise. It's better to declare the specific checked exceptions that can be thrown. getConnection() is declared to throw SQLException and ClassNotFoundException, so the original signature was more accurate.
| void floatArrayPropertyRoundTrip() throws Exception { | |
| void floatArrayPropertyRoundTrip() throws SQLException, ClassNotFoundException { |
|
|
||
| @Test | ||
| void maxFilesRetention() throws IOException { | ||
| void maxFilesRetention() throws Exception { |
There was a problem hiding this comment.
The throws clause was changed from IOException to Exception. It's better to be specific about the exceptions a method can throw. Since createBackupFiles() can throw an IOException, the method signature should declare throws IOException.
| void maxFilesRetention() throws Exception { | |
| void maxFilesRetention() throws IOException { |
|
|
||
| @Test | ||
| void sessionIdleTimeout() throws InterruptedException { | ||
| void sessionIdleTimeout() throws Exception { |
There was a problem hiding this comment.
The throws clause was changed from InterruptedException to Exception. It's better to be specific about the exceptions a method can throw. Since Thread.sleep() throws InterruptedException, the method signature should declare throws InterruptedException.
| void sessionIdleTimeout() throws Exception { | |
| void sessionIdleTimeout() throws InterruptedException { |
🧪 CI InsightsHere's what we observed from your CI run for d80f2b5. 🟢 All jobs passed!But CI Insights is watching 👀 |
(cherry picked from commit 5f4f908)
Bumps [github/codeql-action](https://github.com/github/codeql-action) from 4.31.3 to 4.31.4. Release notes *Sourced from [github/codeql-action's releases](https://github.com/github/codeql-action/releases).* > v4.31.4 > ------- > > CodeQL Action Changelog > ======================= > > See the [releases page](https://github.com/github/codeql-action/releases) for the relevant changes to the CodeQL CLI and language packs. > > 4.31.4 - 18 Nov 2025 > -------------------- > > No user facing changes. > > See the full [CHANGELOG.md](https://github.com/github/codeql-action/blob/v4.31.4/CHANGELOG.md) for more information. Changelog *Sourced from [github/codeql-action's changelog](https://github.com/github/codeql-action/blob/main/CHANGELOG.md).* > CodeQL Action Changelog > ======================= > > See the [releases page](https://github.com/github/codeql-action/releases) for the relevant changes to the CodeQL CLI and language packs. > > [UNRELEASED] > ------------ > > No user facing changes. > > 4.31.4 - 18 Nov 2025 > -------------------- > > No user facing changes. > > 4.31.3 - 13 Nov 2025 > -------------------- > > * CodeQL Action v3 will be deprecated in December 2026. The Action now logs a warning for customers who are running v3 but could be running v4. For more information, see [Upcoming deprecation of CodeQL Action v3](https://github.blog/changelog/2025-10-28-upcoming-deprecation-of-codeql-action-v3/). > * Update default CodeQL bundle version to 2.23.5. [ArcadeData#3288](https://redirect.github.com/github/codeql-action/pull/3288) > > 4.31.2 - 30 Oct 2025 > -------------------- > > No user facing changes. > > 4.31.1 - 30 Oct 2025 > -------------------- > > * The `add-snippets` input has been removed from the `analyze` action. This input has been deprecated since CodeQL Action 3.26.4 in August 2024 when this removal was announced. > > 4.31.0 - 24 Oct 2025 > -------------------- > > * Bump minimum CodeQL bundle version to 2.17.6. [ArcadeData#3223](https://redirect.github.com/github/codeql-action/pull/3223) > * When SARIF files are uploaded by the `analyze` or `upload-sarif` actions, the CodeQL Action automatically performs post-processing steps to prepare the data for the upload. Previously, these post-processing steps were only performed before an upload took place. We are now changing this so that the post-processing steps will always be performed, even when the SARIF files are not uploaded. This does not change anything for the `upload-sarif` action. For `analyze`, this may affect Advanced Setup for CodeQL users who specify a value other than `always` for the `upload` input. [ArcadeData#3222](https://redirect.github.com/github/codeql-action/pull/3222) > > 4.30.9 - 17 Oct 2025 > -------------------- > > * Update default CodeQL bundle version to 2.23.3. [ArcadeData#3205](https://redirect.github.com/github/codeql-action/pull/3205) > * Experimental: A new `setup-codeql` action has been added which is similar to `init`, except it only installs the CodeQL CLI and does not initialize a database. Do not use this in production as it is part of an internal experiment and subject to change at any time. [ArcadeData#3204](https://redirect.github.com/github/codeql-action/pull/3204) > > 4.30.8 - 10 Oct 2025 > -------------------- > > No user facing changes. > > 4.30.7 - 06 Oct 2025 > -------------------- > > * [v4+ only] The CodeQL Action now runs on Node.js v24. [ArcadeData#3169](https://redirect.github.com/github/codeql-action/pull/3169) > > 3.30.6 - 02 Oct 2025 > -------------------- > > * Update default CodeQL bundle version to 2.23.2. [ArcadeData#3168](https://redirect.github.com/github/codeql-action/pull/3168) > > 3.30.5 - 26 Sep 2025 > -------------------- > > * We fixed a bug that was introduced in `3.30.4` with `upload-sarif` which resulted in files without a `.sarif` extension not getting uploaded. [ArcadeData#3160](https://redirect.github.com/github/codeql-action/pull/3160) ... (truncated) Commits * [`e12f017`](github/codeql-action@e12f017) Merge pull request [ArcadeData#3312](https://redirect.github.com/github/codeql-action/issues/3312) from github/update-v4.31.4-70434f6dd * [`c9cb6f9`](github/codeql-action@c9cb6f9) Update changelog for v4.31.4 * [`70434f6`](github/codeql-action@70434f6) Merge pull request [ArcadeData#3311](https://redirect.github.com/github/codeql-action/issues/3311) from github/mbg/deps/bump-glob * [`528362a`](github/codeql-action@528362a) Bump `glob` to at least `11.1.0` * [`de12435`](github/codeql-action@de12435) Merge pull request [ArcadeData#3308](https://redirect.github.com/github/codeql-action/issues/3308) from github/mbg/pr-template/nov25 * [`ffa63f0`](github/codeql-action@ffa63f0) Merge pull request [ArcadeData#3307](https://redirect.github.com/github/codeql-action/issues/3307) from github/dependabot/github\_actions/dot-github/wor... * [`7bcdb4b`](github/codeql-action@7bcdb4b) Add additional options to PR template and clarify some * [`07eae64`](github/codeql-action@07eae64) Merge pull request [ArcadeData#3303](https://redirect.github.com/github/codeql-action/issues/3303) from github/mario-campos/v3-core-warning * [`e546fff`](github/codeql-action@e546fff) Rebuild * [`c418a0f`](github/codeql-action@c418a0f) Bump ruby/setup-ruby * Additional commits viewable in [compare view](github/codeql-action@014f16e...e12f017) [](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- Dependabot commands and options You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot show ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
What does this PR do?
fixes FQNs to import
fixes junit assertions to assertj