AB-449 Fix flaky tests and properly clean up resources by ThoSap · Pull Request #7 · aboutbits/postgresql-operator

ThoSap · 2026-01-26T18:24:06Z

No description provided.

linear · 2026-01-26T18:24:09Z

Based on the research and PoC done in the ticket and PR https://github.com/aboutbits/minio-operator/pull/1

We most likely will use CloudNativePG AB-396 for the DB Setup and this operator to manage the access, e.g. PostgreSQL roles and grants.

The scope of the operator is:

Database/Schema management
Role management
Grant/Default Privilege management

… for now

Copilot

Pull request overview

This PR addresses flaky test issues and improves test resource cleanup by implementing proper deletion polling and resource dependency management.

Changes:

Replaced synchronous resource deletion with async polling to ensure complete cleanup
Added explicit ReclaimPolicy.DELETE to test resources to ensure proper cleanup
Fixed test data creators to properly chain resource dependencies

Reviewed changes

Copilot reviewed 29 out of 29 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
`operator/src/test/java/it/aboutbits/postgresql/crd/schema/SchemaReconcilerTest.java`	Updated cleanup to use await polling instead of timeouts
`operator/src/test/java/it/aboutbits/postgresql/crd/role/RoleReconcilerTest.java`	Added dependent resource cleanup order and reduced poll intervals
`operator/src/test/java/it/aboutbits/postgresql/crd/grant/GrantReconcilerTest.java`	Added manual table cleanup, disabled slow tests, and improved assertion patterns
`operator/src/test/java/it/aboutbits/postgresql/crd/defaultprivilege/DefaultPrivilegeReconcilerTest.java`	Applied consistent cleanup pattern and disabled slow parameterized tests
`operator/src/test/java/it/aboutbits/postgresql/crd/database/DatabaseReconcilerTest.java`	Updated cleanup to use await polling
`operator/src/test/java/it/aboutbits/postgresql/crd/clusterconnection/ClusterConnectionReconcilerTest.java`	Updated cleanup to use await polling
`operator/src/test/java/it/aboutbits/postgresql/_support/testdata/persisted/creator/SecretRefCreate.java`	Fixed field caching to ensure consistent values across test data creation
`operator/src/test/java/it/aboutbits/postgresql/_support/testdata/persisted/creator/SchemaCreate.java`	Updated to create cluster connections dynamically instead of assuming default
`operator/src/test/java/it/aboutbits/postgresql/_support/testdata/persisted/creator/RoleCreate.java`	Updated to create cluster connections dynamically
`operator/src/test/java/it/aboutbits/postgresql/_support/testdata/persisted/creator/GrantCreate.java`	Added proper dependency chain creation for database, role, and schema resources
`operator/src/test/java/it/aboutbits/postgresql/_support/testdata/persisted/creator/DefaultPrivilegeCreate.java`	Added proper dependency chain creation
`operator/src/test/java/it/aboutbits/postgresql/_support/testdata/persisted/creator/DatabaseCreate.java`	Updated to create cluster connections dynamically
`operator/src/test/java/it/aboutbits/postgresql/_support/testdata/persisted/creator/ClusterConnectionCreate.java`	Fixed field caching to ensure consistent values
`operator/src/test/java/it/aboutbits/postgresql/_support/testdata/persisted/Given.java`	Updated factory methods to pass Given instance to creators
`operator/src/test/java/it/aboutbits/postgresql/_support/testdata/base/TestDataCreator.java`	Fixed random name suffix generation to handle edge cases
`operator/src/test/java/it/aboutbits/postgresql/PostgreSQLInstanceReadinessCheckTest.java`	Updated cleanup to use await polling
`operator/src/main/java/it/aboutbits/postgresql/crd/schema/SchemaSpec.java`	Enhanced validation to trim whitespace before checking emptiness
`operator/src/main/java/it/aboutbits/postgresql/crd/role/RoleSpec.java`	Enhanced validation to trim whitespace before checking emptiness
`operator/src/main/java/it/aboutbits/postgresql/crd/grant/GrantReconciler.java`	Added checkstyle suppression for method length
`operator/src/main/java/it/aboutbits/postgresql/crd/defaultprivilege/DefaultPrivilegeService.java`	Fixed missing owner filter in default privilege query
`operator/src/main/java/it/aboutbits/postgresql/crd/defaultprivilege/DefaultPrivilegeReconciler.java`	Fixed cleanup to use correct database context
`operator/src/main/java/it/aboutbits/postgresql/crd/database/DatabaseSpec.java`	Enhanced validation to trim whitespace before checking emptiness
`operator/src/main/java/it/aboutbits/postgresql/crd/clusterconnection/ClusterConnectionSpec.java`	Enhanced validation to trim whitespace before checking emptiness
`operator/src/main/java/it/aboutbits/postgresql/crd/clusterconnection/ClusterConnectionReconciler.java`	Added reconciliation logging
`operator/src/main/java/it/aboutbits/postgresql/core/SecretRef.java`	Enhanced validation to trim whitespace before checking emptiness
`operator/src/main/java/it/aboutbits/postgresql/core/ClusterReference.java`	Enhanced validation to trim whitespace before checking emptiness
`gradle/libs.versions.toml`	Updated nullAway version from 0.13.0 to 0.13.1
`.github/workflows/test.yml`	Increased test timeout from 5 to 10 minutes
`.github/workflows/release.yml`	Decreased release timeout from 15 to 5 minutes

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-27T09:19:46Z

operator/src/test/java/it/aboutbits/postgresql/crd/schema/SchemaReconcilerTest.java

-        kubernetesClient.resources(ClusterConnection.class)
-                .withTimeout(5, TimeUnit.SECONDS)
-                .delete();
+    void resetEnvironment() {


The method is renamed from cleanUp to resetEnvironment, but this naming is inconsistent with its actual behavior. The method performs cleanup operations but is called 'reset'. Consider using a name like cleanUpResources or keeping the original cleanUp name for consistency and clarity.

Suggested change

void resetEnvironment() {

void cleanUp() {

Copilot · 2026-01-27T09:19:47Z

operator/src/test/java/it/aboutbits/postgresql/_support/testdata/base/TestDataCreator.java

+        var availableSpace = maxLength - name.length() - separator.length();
+        var suffixLength = Math.min(7, availableSpace);


If availableSpace is negative (when name + separator exceeds maxLength), this will result in a negative value being passed to the regex pattern on line 68, causing a runtime exception. Add validation to handle cases where the name is too long.

What are you talking about, that logic is already present a few lines higher...

Copilot · 2026-01-27T09:19:47Z

...r/src/main/java/it/aboutbits/postgresql/crd/defaultprivilege/DefaultPrivilegeReconciler.java

        var clusterConnection = clusterConnectionOptional.get();

-        try (var dsl = contextFactory.getDSLContext(clusterConnection)) {
+        try (var dsl = contextFactory.getDSLContext(clusterConnection, database)) {


The cleanup method now connects to a specific database rather than the default connection. If the database doesn't exist or is inaccessible during cleanup, this could cause the cleanup to fail. Consider handling potential connection failures gracefully.

We already handle any errors in the CRD and show the error in the CRStatus and logs.
I don't think there's a graceful way to handle a missing database when it was deleted from the RDBMS.

.../src/test/java/it/aboutbits/postgresql/_support/testdata/persisted/creator/SchemaCreate.java

...or/src/test/java/it/aboutbits/postgresql/_support/testdata/persisted/creator/RoleCreate.java

...r/src/test/java/it/aboutbits/postgresql/_support/testdata/persisted/creator/GrantCreate.java

...java/it/aboutbits/postgresql/_support/testdata/persisted/creator/DefaultPrivilegeCreate.java

...rc/test/java/it/aboutbits/postgresql/_support/testdata/persisted/creator/DatabaseCreate.java

Copilot

Pull request overview

Copilot reviewed 29 out of 29 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-27T09:37:13Z

...r/src/test/java/it/aboutbits/postgresql/_support/testdata/persisted/creator/GrantCreate.java

+
+        var clusterConnection = given.one()
+                .clusterConnection()
+                .withName("conn-%s".formatted(getName()))


The variable withName may be null when getClusterConnectionName() is called, resulting in a NullPointerException. The method should call getName() instead to ensure the name is generated if not already set.

Suggested change

.withName("conn-%s".formatted(getName()))

.withName("conn-%s".formatted(getName()))

WTF Bot!

You shall be replaced by Claude 🤓

Copilot

Pull request overview

Copilot reviewed 29 out of 29 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-27T09:40:12Z

...ator/src/main/java/it/aboutbits/postgresql/crd/defaultprivilege/DefaultPrivilegeService.java

            DSLContext tx,
            DefaultPrivilegeSpec spec
    ) {
+        var owner = spec.getOwner();


The owner variable is extracted but not used in the subsequent code logic. The owner filter condition is added later in the WHERE clause but references spec.getOwner() directly. This variable declaration appears redundant.

WTF is wrong with you! 😂😂😂😂
What is the newly added usage a few lines below then?

operator/src/test/java/it/aboutbits/postgresql/crd/role/RoleReconcilerTest.java

Copilot

Pull request overview

Copilot reviewed 29 out of 29 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

...c/test/java/it/aboutbits/postgresql/_support/testdata/persisted/creator/SecretRefCreate.java

Copilot

Pull request overview

Copilot reviewed 29 out of 29 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-27T09:57:28Z

operator/src/test/java/it/aboutbits/postgresql/crd/role/RoleReconcilerTest.java

+        kubernetesClient.resource(role).delete();
+        role.getMetadata().setFinalizers(null);
+        role.getMetadata().setResourceVersion(null);
+        kubernetesClient.resource(role).patch();


This manual cleanup workaround is necessary because the ClusterConnection doesn't exist, but it's fragile and could break if the Kubernetes API behavior changes. Consider refactoring the test to avoid creating resources with missing dependencies, or extract this cleanup logic into a test utility method with documentation explaining why it's needed.

That is the point of the test, to trigger an error due to a missing dependency...
I wonder what happened with you, are ya' drunk? 🧐

…R in the reverse dependency order, so we prevent possible cross-test pollution

Copilot

Pull request overview

Copilot reviewed 34 out of 34 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

operator/src/test/java/it/aboutbits/postgresql/crd/grant/GrantReconcilerTest.java

Copilot

Pull request overview

Copilot reviewed 33 out of 33 changed files in this pull request and generated no new comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

… creator

This reverts commit 0792764.

Copilot

Pull request overview

Copilot reviewed 33 out of 33 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

...r/src/test/java/it/aboutbits/postgresql/_support/testdata/persisted/creator/GrantCreate.java

...java/it/aboutbits/postgresql/_support/testdata/persisted/creator/DefaultPrivilegeCreate.java

mmalfertheiner

Nice Job Thomas!

fix flaky tests and properly clean up resources

a121886

ThoSap self-assigned this Jan 26, 2026

ThoSap marked this pull request as draft January 26, 2026 18:33

ThoSap added 3 commits January 26, 2026 19:49

disable some slow and flaky tests (when running the whole test suite)…

1f7bd23

… for now

add additional CRD CEL String length validations

454a4b1

add additional null checks as this check is sometimes flaky on CI

3423a1e

ThoSap force-pushed the ab-449-fix-flaky-tests-and-properly-clean-up-resources branch from f60a5db to 3423a1e Compare January 27, 2026 06:28

ThoSap added 4 commits January 27, 2026 08:37

update NullAway to 0.13.1

d400d37

fix some disabled tests and test data creators

7649ee5

use assertThatExceptionOfType instead of assertThatThrownBy

491097a

fix a flaky test by cleaning additional CR

793901a

ThoSap requested a review from Copilot January 27, 2026 09:16