Add some failing test cases for DSN environment variable expansion.

[ggreer]

Some test cases that fail now that #92 is merged.

Summary by CodeRabbit

• Tests
  • Extended test coverage for DSN expansion scenarios, including environment variable substitution, component replacement, and placeholder handling.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

This pull request extends test coverage for DSN expansion by adding new test cases to Test_expandDSN. The additions validate various DSN expansion scenarios including environment variable substitution, scheme handling, placeholder replacement, and escaping behavior. No production code is modified.

Changes

Cohort / File(s)	Summary
Test coverage expansion pkg/database/database_test.go	Adds new test cases to Test_expandDSN covering fully specified DSN via environment variables, DSN components substituted from environment (endpoint, scheme), single and multiple variable replacements, and multiple placeholder combinations

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

• Review focuses on validating test case correctness and expected behavior coverage
• Verify new test scenarios align with existing expansion logic

Possibly related PRs

• Improve DSN env vairable expansion #92 — Also modifies Test_expandDSN in pkg/database/database_test.go with additional DSN expansion test cases

Suggested reviewers

• laurenleach
• btipling
• pquerna

Poem

🐰 A tester's delight, with DSN so bright,
Expanding variables left and right,
With schemes and endpoints taking their flight,
Each placeholder escapes just right!
More coverage brings joy to the night, ✨

Pre-merge checks and finishing touches

❌ 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%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main change: adding new test cases for DSN environment variable expansion.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch ggreer/expand-env-tests

---

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

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

pkg/database/database_test.go (1)

86-520: Consider migrating to testify/require for better test assertions.

The coding guidelines specify that tests should use testify/require, but this test file uses the standard library's testing package (t.Errorf). While the new test cases are consistent with the existing style in this file, consider refactoring all tests to use require.NoError, require.Equal, etc., for clearer assertion failures and better test output.

As per coding guidelines

Example refactor for one test case:

+import (
+	"github.com/stretchr/testify/require"
+)

 func Test_expandDSN(t *testing.T) {
 	// ... test setup ...
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			for k, v := range tt.env {
 				t.Setenv(k, v)
 			}
 
 			got, err := expandDSN(tt.dsn)
-			if (err != nil) != tt.wantErr {
-				t.Errorf("expandDSN() error = %v, wantErr %v", err, tt.wantErr)
-				return
-			}
-			if got != tt.want {
-				t.Errorf("expandDSN() got = %v, want %v", got, tt.want)
-			}
+			if tt.wantErr {
+				require.Error(t, err)
+			} else {
+				require.NoError(t, err)
+				require.Equal(t, tt.want, got)
+			}
 		})
 	}
 }

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5879591 and f2f55ab.

📒 Files selected for processing (1)

• pkg/database/database_test.go (1 hunks)

🧰 Additional context used

📓 Path-based instructions (2)

**/*.go

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.go: Error handling: use fmt.Errorf with contextual messages; check specific errors with errors.Is
Organize imports: standard library first, then third-party, then project imports; alphabetize within each group
Naming: CamelCase for exported identifiers; camelCase for unexported; preserve acronyms like ID, URL, HTTP, API
Limit line length to a maximum of 200 characters
Comments for exported items must be complete sentences ending with periods
Do not use log.Fatal or log.Panic (ruleguard-enforced)

Files:

• pkg/database/database_test.go

**/*_test.go

📄 CodeRabbit inference engine (CLAUDE.md)

Tests should be table-driven using testify/require; name tests TestStructName_methodName

Files:

• pkg/database/database_test.go

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: go-test (ubuntu-latest)

🔇 Additional comments (2)

pkg/database/database_test.go (2)

255-278: LGTM! Good coverage of DSN expansion scenarios.

These test cases appropriately cover:

• Full DSN from a single environment variable
• Partial DSN substitution (endpoint)
• Scheme substitution

The test structure is consistent with existing tests in the file.

---

279-295: Use testify/require for assertions and clarify what sentinel collision behavior is being tested.

These test cases at lines 284 and 290 expose a real sentinel collision vulnerability:

• Line 284: DSN contains literal "db-999000" which collides with a sentinel value
• Line 290: Env var contains "user-999001" which will be used as a replacement value and then incorrectly re-replaced when sentinel "999001" is processed

The root cause is in expandWithMapping() (database.go:97-102): it uses strings.ReplaceAll() indiscriminately, so when a value being expanded contains a numeric sentinel pattern, it gets replaced again during subsequent iterations.

Additionally, this test function does not use testify/require for assertions as required by the coding guidelines. Replace t.Errorf() calls with require.Equal() to match the project standard.