Skip to content

Conversation

@MayurMallavSaikia
Copy link

@MayurMallavSaikia MayurMallavSaikia commented Oct 12, 2025

Test stability and correctness

  • Add a small sleep and a timeout in the “wait until DB is available” loop to avoid busy-waiting forever on a failing setup.

  • Fix assertEquals argument order (expected first, then actual) in TestRDS.

Code hygiene

  • Use try-with-resources for Statement and ResultSet in RDS.test_connection to ensure they are reliably closed.

Dependency updates and cleanups (in pom.xml)

  • Remove duplicate software.amazon.awssdk:s3 entry.

  • Remove unused AWS SDK v1 test dependency (com.amazonaws:aws-java-sdk-s3). Project already uses AWS SDK v2.

  • Update JUnit 4 to 4.13.2 (latest 4.x) to address old CVEs.

  • Update PostgreSQL JDBC driver to a modern version (e.g., 42.7.4).

  • Optionally update Testcontainers LocalStack module and AWS SDK v2 BOM to recent stable versions.

README

  • Add direct CLI instructions to run the test without an IDE (mvn -q -Dtest=TestRDS test).

  • Explicitly document prerequisites (Docker running, LocalStack Pro token) and how credentials are provided in the test.

@MayurMallavSaikia MayurMallavSaikia marked this pull request as ready for review October 12, 2025 06:41
Copy link

@blkgrlcto blkgrlcto left a comment

Choose a reason for hiding this comment

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

Nice modernization pass. The dependency updates, improved README, smarter wait loop, and corrected JUnit assert order all look great.

One small issue in RDS.java:
The connection closing logic in the finally block is redundant and could throw a NullPointerException. Right now it calls c.close(); and then if (c != null) c.close();, which means the first call can fail if c is null, and both are unnecessary.

You can fix this by either:

  • Keeping just one null-checked c.close() inside the finally block, or
  • Wrapping the Connection in a try-with-resources statement and removing the finally entirely (preferred, since the file already uses try-with-resources for Statement and ResultSet).

@MayurMallavSaikia
Copy link
Author

Nice modernization pass. The dependency updates, improved README, smarter wait loop, and corrected JUnit assert order all look great.

One small issue in RDS.java: The connection closing logic in the finally block is redundant and could throw a NullPointerException. Right now it calls c.close(); and then if (c != null) c.close();, which means the first call can fail if c is null, and both are unnecessary.

You can fix this by either:

  • Keeping just one null-checked c.close() inside the finally block, or
  • Wrapping the Connection in a try-with-resources statement and removing the finally entirely (preferred, since the file already uses try-with-resources for Statement and ResultSet).

Hi @blkgrlcto , Thanks for catching that! The redundant c.close() call was from an earlier revision — the current version only has the null-checked close inside the finally block, so it won’t throw a NullPointerException anymore. Appreciate the thorough review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants