NO-ISSUE: Move common test database setup to the suite level

[jhernand]

Summary

• Extracts the duplicated per-test database setup (context creation, database instance, connection
  pool, transaction manager, and transaction) from every server test file into a single global
  BeforeEach in servers_suite_test.go, promoting ctx to a package-level variable.
• Tests that need direct transaction access for raw SQL (e.g. adding finalizers) now obtain it via
  database.TxFromContext(ctx) instead of relying on a local tx variable.
• Removes approximately 1,100 lines of duplicated boilerplate across 36 test files.

Test plan

• Verify ginkgo run -r internal passes with no regressions.

Summary by CodeRabbit

• Tests

  • Centralized and simplified test harness: per-test context/transaction setup moved to shared suite-level setup, and individual server tests no longer duplicate DB/transaction initialization. Tests now obtain the active transaction from the shared context where needed.

• Chores

  • Removed numerous unused imports from test files and tidied test scaffolding.

[openshift-ci-robot]

@jhernand: This pull request explicitly references no jira issue.

Details

In response to this:

Summary

• Extracts the duplicated per-test database setup (context creation, database instance, connection
  pool, transaction manager, and transaction) from every server test file into a single global
  BeforeEach in servers_suite_test.go, promoting ctx to a package-level variable.
• Tests that need direct transaction access for raw SQL (e.g. adding finalizers) now obtain it via
  database.TxFromContext(ctx) instead of relying on a local tx variable.
• Removes approximately 1,100 lines of duplicated boilerplate across 36 test files.

Test plan

• Verify ginkgo run -r internal passes with no regressions.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jhernand

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [jhernand]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: 7ffbec17-8620-4bd0-8d3c-b8e7fb241b74

📥 Commits

Reviewing files that changed from the base of the PR and between 701183b and 5be0077.

📒 Files selected for processing (36)

• internal/controllers/computeinstance/computeinstance_reconciler_function_test.go
• internal/servers/cluster_catalog_items_server_test.go
• internal/servers/compute_instances_server_test.go
• internal/servers/generic_mapper_test.go
• internal/servers/host_types_server_test.go
• internal/servers/network_classes_server_test.go
• internal/servers/organizations_server_test.go
• internal/servers/private_cluster_catalog_items_server_test.go
• internal/servers/private_cluster_templates_server_test.go
• internal/servers/private_clusters_server_test.go
• internal/servers/private_compute_instance_catalog_items_server_test.go
• internal/servers/private_compute_instance_templates_server_test.go
• internal/servers/private_compute_instances_server_test.go
• internal/servers/private_host_types_server_test.go
• internal/servers/private_hubs_server_test.go
• internal/servers/private_organizations_server_test.go
• internal/servers/private_projects_server_test.go
• internal/servers/private_public_ip_attachments_server_test.go
• internal/servers/private_public_ip_pools_server_test.go
• internal/servers/private_public_ips_server_test.go
• internal/servers/private_role_bindings_server_test.go
• internal/servers/private_roles_server_test.go
• internal/servers/private_subnets_server_test.go
• internal/servers/private_users_server_test.go
• internal/servers/private_virtual_networks_server_test.go
• internal/servers/projects_server_test.go
• internal/servers/public_ip_attachments_server_test.go
• internal/servers/public_ips_server_test.go
• internal/servers/role_bindings_server_test.go
• internal/servers/roles_server_test.go
• internal/servers/security_groups_server_test.go
• internal/servers/servers_suite_test.go
• internal/servers/servers_tenancy_test.go
• internal/servers/subnets_server_test.go
• internal/servers/users_server_test.go
• internal/servers/virtual_networks_server_test.go

💤 Files with no reviewable changes (14)

• internal/servers/generic_mapper_test.go
• internal/servers/private_cluster_templates_server_test.go
• internal/servers/private_public_ip_attachments_server_test.go
• internal/controllers/computeinstance/computeinstance_reconciler_function_test.go
• internal/servers/private_hubs_server_test.go
• internal/servers/private_compute_instance_templates_server_test.go
• internal/servers/private_public_ips_server_test.go
• internal/servers/private_cluster_catalog_items_server_test.go
• internal/servers/private_host_types_server_test.go
• internal/servers/private_public_ip_pools_server_test.go
• internal/servers/private_compute_instance_catalog_items_server_test.go
• internal/servers/servers_tenancy_test.go
• internal/servers/private_clusters_server_test.go
• internal/servers/organizations_server_test.go

---

Walkthrough

Centralizes per-test DB/transaction wiring into internal/servers/servers_suite_test.go (per-test BeforeEach that builds DB/pool, begins a tx, and injects it into ctx). Individual server test files remove duplicate ctx/tx setup and now either rely on the suite-provided ctx or call database.TxFromContext(ctx) before executing raw SQL.

Changes

Server test harness centralization

Layer / File(s)	Summary
Suite-level context and transaction setup internal/servers/servers_suite_test.go	Adds suite ctx and a per-test BeforeEach that creates DB instance/pool, TxManager, begins a transaction, injects tx into ctx via database.TxIntoContext, and defers cleanup; removes the old visibility BeforeSuite.
Per-file test scaffolding simplification internal/servers/*_server_test.go (many files)	Many server test files remove local context / internal/database imports, delete Describe-scoped ctx/tx and their BeforeEach transaction setup, and proceed directly to build servers/DAOs using the suite-provided ctx.
SQL tests fetch tx at runtime internal/servers/cluster_catalog_items_server_test.go, internal/servers/network_classes_server_test.go, internal/servers/host_types_server_test.go, internal/servers/public_ip_attachments_server_test.go, internal/servers/public_ips_server_test.go, internal/servers/subnets_server_test.go, internal/servers/virtual_networks_server_test.go, internal/servers/private_virtual_networks_server_test.go, internal/servers/security_groups_server_test.go	Tests that execute raw SQL now call database.TxFromContext(ctx) and assert no error before running finalizer/index/soft-delete updates, replacing reliance on hoisted tx variables.
Compute instance / controller test import tweaks internal/controllers/computeinstance/computeinstance_reconciler_function_test.go, internal/servers/compute_instances_server_test.go, internal/servers/private_compute_instances_server_test.go	Minor import reordering/removals: removed unused proto in a controller test; compute/private compute instance tests adjust imports (add internal/database/dao and add proto in private compute instances test).

Sequence Diagram

sequenceDiagram
  participant ServerSuite
  participant Database
  participant TxManager
  participant TestCase
  ServerSuite->>Database: build instance & pool
  ServerSuite->>TxManager: create tx manager
  TxManager->>Database: begin transaction
  ServerSuite->>TestCase: inject tx into ctx (database.TxIntoContext)
  TestCase->>Database: reuse ctx-backed tx or call database.TxFromContext(ctx)

Loading

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• osac-project/fulfillment-service#617: Related protobuf/import adjustments in controller tests.
• osac-project/fulfillment-service#585: Overlapping test scaffolding and DB setup changes in server tests.
• osac-project/fulfillment-service#565: Prior controller/test changes touching same test files.

Suggested reviewers

• adriengentil
• eranco74

"Suite spins up DB and Tx with care,
Tests now share context everywhere,
No more hoisted tx in every file—
One setup, many tests, and CI will smile."

🚥 Pre-merge checks | ✅ 10 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (10 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main objective: consolidating duplicated per-test database setup from 36 test files into a suite-level BeforeEach in servers_suite_test.go.
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.
No-Hardcoded-Secrets	✅ Passed	No hardcoded secrets detected. Test fixtures like "secret123" are intentional test data used to verify credential redaction logic, not production credentials.
No-Weak-Crypto	✅ Passed	PR contains no weak cryptography (MD5, SHA1, DES, RC4, etc.), custom crypto implementations, or timing-vulnerable comparisons. This is a test-refactoring PR with no cryptographic code changes.
No-Injection-Vectors	✅ Passed	All SQL queries use parameterized statements with $1, $2 placeholders. No shell, eval, pickle, yaml.load, or XSS injection vectors detected in the refactored test code.
Container-Privileges	✅ Passed	PR contains only Go test file refactoring; no Kubernetes manifests, Dockerfiles, or container security configurations were modified, so privileged settings check is not applicable.
No-Sensitive-Data-In-Logs	✅ Passed	Database passwords use AddSecrets() redaction, all logging uses redaction-enabled slog.Logger, no sensitive data is directly exposed in logs.
Ai-Attribution	✅ Passed	PR commit 5be0077 properly attributes AI assistance with "Assisted-by: Cursor" trailer; no improper Co-Authored-By usage detected for AI tools.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@internal/servers/host_types_server_test.go`:
- Around line 229-230: The test calls database.TxFromContext(ctx) and
immediately uses tx.Exec without checking err; update the test to assert or fail
if err != nil before dereferencing tx (i.e., after calling TxFromContext and
before calling tx.Exec) so the test fails with a clear message instead of
panicking; reference the call to database.TxFromContext(ctx) and the tx variable
in the host_types_server_test test to locate and fix the assertion.

In `@internal/servers/servers_suite_test.go`:
- Around line 97-98: BeforeEach currently does ctx = context.Background() which
lacks a deadline; change it to create a cancellable context with a timeout (use
context.WithTimeout(context.Background(), <reasonable duration>)) and assign
returned ctx and cancel to the test-scoped variables (ctx and cancel). Ensure
the corresponding AfterEach calls cancel() to release resources and prevent test
hangs; update imports to include time if needed. Locate the setup in BeforeEach
and the cleanup in AfterEach in servers_suite_test.go and modify those functions
(ctx variable, BeforeEach, AfterEach) accordingly.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: b6942d95-f659-4bd5-a5e6-474b59e2e280

📥 Commits

Reviewing files that changed from the base of the PR and between cc0688d and 701183b.

📒 Files selected for processing (36)

• internal/controllers/computeinstance/computeinstance_reconciler_function_test.go
• internal/servers/cluster_catalog_items_server_test.go
• internal/servers/compute_instances_server_test.go
• internal/servers/generic_mapper_test.go
• internal/servers/host_types_server_test.go
• internal/servers/network_classes_server_test.go
• internal/servers/organizations_server_test.go
• internal/servers/private_cluster_catalog_items_server_test.go
• internal/servers/private_cluster_templates_server_test.go
• internal/servers/private_clusters_server_test.go
• internal/servers/private_compute_instance_catalog_items_server_test.go
• internal/servers/private_compute_instance_templates_server_test.go
• internal/servers/private_compute_instances_server_test.go
• internal/servers/private_host_types_server_test.go
• internal/servers/private_hubs_server_test.go
• internal/servers/private_organizations_server_test.go
• internal/servers/private_projects_server_test.go
• internal/servers/private_public_ip_attachments_server_test.go
• internal/servers/private_public_ip_pools_server_test.go
• internal/servers/private_public_ips_server_test.go
• internal/servers/private_role_bindings_server_test.go
• internal/servers/private_roles_server_test.go
• internal/servers/private_subnets_server_test.go
• internal/servers/private_users_server_test.go
• internal/servers/private_virtual_networks_server_test.go
• internal/servers/projects_server_test.go
• internal/servers/public_ip_attachments_server_test.go
• internal/servers/public_ips_server_test.go
• internal/servers/role_bindings_server_test.go
• internal/servers/roles_server_test.go
• internal/servers/security_groups_server_test.go
• internal/servers/servers_suite_test.go
• internal/servers/servers_tenancy_test.go
• internal/servers/subnets_server_test.go
• internal/servers/users_server_test.go
• internal/servers/virtual_networks_server_test.go

💤 Files with no reviewable changes (14)

• internal/controllers/computeinstance/computeinstance_reconciler_function_test.go
• internal/servers/private_clusters_server_test.go
• internal/servers/private_public_ips_server_test.go
• internal/servers/generic_mapper_test.go
• internal/servers/private_compute_instance_catalog_items_server_test.go
• internal/servers/organizations_server_test.go
• internal/servers/servers_tenancy_test.go
• internal/servers/private_cluster_templates_server_test.go
• internal/servers/private_compute_instance_templates_server_test.go
• internal/servers/private_public_ip_attachments_server_test.go
• internal/servers/private_host_types_server_test.go
• internal/servers/private_hubs_server_test.go
• internal/servers/private_public_ip_pools_server_test.go
• internal/servers/private_cluster_catalog_items_server_test.go