Skip to content

test: add Orm.Connection dbConfig coverage and optimize tests/feature runtime (framework#1540) - #147

Merged
hwbrzzl merged 2 commits into
masterfrom
bowen/test-pr-1540-orm-connection-dbconfig
Sep 1, 2026
Merged

test: add Orm.Connection dbConfig coverage and optimize tests/feature runtime (framework#1540)#147
hwbrzzl merged 2 commits into
masterfrom
bowen/test-pr-1540-orm-connection-dbconfig

Conversation

@goravel-coder

@goravel-coder goravel-coder commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

Summary

  • facades.Orm().Connection(name) now returns an ORM bound to the requested connection's own config on every call — the cached/warm path and 20 concurrent callers no longer leak the default connection's name and database; coverage exercises the cold, warm, and concurrent paths against the framework#1540 fix build.
  • Migrations targeting a non-default connection create their tables there while the migrations ledger is written to the default connection only; the new test asserts the reporting DB has the tables but no ledger table, and both ledger rows land on the default DB.
  • The tests/feature suite runtime drops from ~600s to ~400s: the driver-agnostic Orm/migrator tests now run only under sqlite (they were re-run redundantly by TestDBDrivers for postgres/mysql/sqlserver), the broadcast suite's queue.default: "database" override is hoisted to SetupSuite (saving ~18 app restarts), sync-reliant tests opt back into the sync queue, fixed-sleep job-consumption checks became Eventually polls, and workers get a deferred shutdown to avoid goroutine leaks.

Closes https://github.com/goravel/goravel/issues/1540

Why

Framework#1540 fixes two ways a non-default database connection could leak into shared state. Orm.Connection cached the ORM per connection name but always returned the parent dbConfig, so a second Connection("reporting") call reported the default connection's name and database; and the migrator logged its ledger row while still switched to the migration's target connection, so the ledger landed in the wrong database. This branch pins the example to the fix build and adds feature coverage that locks both behaviors down.

// Before: the cached Connection("reporting") path reported the default
// connection's config, so DatabaseName()/Config() pointed at the wrong DB.
reporting := facades.Orm().Connection("reporting")

// After: every call returns an ORM bound to the requested connection.
reporting := facades.Orm().Connection("reporting")
if err := reporting.Query().Create(&models.User{Name: "bowen"}); err != nil {
	return ctx.Response().Json(http.StatusInternalServerError, http.Json{"error": err.Error()})
}
// Before: a migration targeting "reporting" recorded its ledger row on the
// wrong database — repository.Log ran while switched to "reporting".
func (r *M20260826160940CreateUsers) Connection() string { return "reporting" }

func (r *M20260826160940CreateUsers) Up() error {
	return facades.Schema().Connection("reporting").Create("users", func(table schema.Blueprint) {
		table.String("name")
	})
}

// After: tables are created on "reporting" while the ledger row is written
// to the default connection.

While working in tests/feature the suite was pushing past the CI -timeout 10m. The change scopes the three driver-agnostic Orm/migrator tests to sqlite only, hoists the broadcast suite's queue.default: "database" override into SetupSuite to drop ~18 app.Restart() calls, opts the three sync-reliant full-flow tests back into the sync queue, and replaces fixed-sleep job-consumption assertions with Eventually polling plus deferred worker shutdown. No assertion was weakened; the suite now completes in ~400s.

@goravel-coder
goravel-coder requested a review from a team as a code owner August 30, 2026 03:14
- Scope the 3 driver-agnostic Orm.Connection/migrator tests to sqlite only
  (they were re-run redundantly under postgres/mysql/sqlserver by TestDBDrivers).
- Hoist the broadcast suite's queue.default=database override to SetupSuite,
  dropping ~18 app.Restart() calls; opt 3 sync-reliant tests back into sync.
- Convert fixed-sleep job-consumption assertions to Eventually polling and
  defer worker.Shutdown() to avoid goroutine leaks.
- Tighten disabled_runners wait windows (3s/2s -> 1s/1s).
@goravel-coder goravel-coder changed the title test: add Orm.Connection dbConfig poisoning coverage (framework#1540) test: add Orm.Connection dbConfig coverage and optimize tests/feature runtime (framework#1540) Sep 1, 2026
@hwbrzzl
hwbrzzl merged commit 58a703f into master Sep 1, 2026
9 checks passed
@hwbrzzl
hwbrzzl deleted the bowen/test-pr-1540-orm-connection-dbconfig branch September 1, 2026 08:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants