Describe the Bug
Two issues in internal/pb/v1/models/base_test.go, surfaced by CodeRabbit reviewing #517 — both predate that PR, they just moved location (was portal-backend/v1/models/base_test.go).
-
Discarded errors (lines ~29-30 and ~63-64): both TestBaseModel_BeforeCreate and TestBaseModel_BeforeUpdate call db.AutoMigrate(&TestModel{}) and defer db.Migrator().DropTable(&TestModel{}) without checking their errors. This is flagged by errcheck and blocks the lint gate; a failed migration can also produce a confusing downstream Create/Save failure instead of a clear migration error.
-
Self-fulfilling assertion (lines ~76-89): TestBaseModel_BeforeUpdate's subtest sets model.UpdatedAt = explicitLaterTime directly before calling db.Save(&model), then asserts the reloaded value equals (or is after) that same explicitly-set value. Since the test pre-sets the exact field the BeforeUpdate hook is supposed to set, the assertion passes whether or not the hook actually runs — it doesn't verify the hook's lifecycle behavior at all.
Expected Behavior
- Require
AutoMigrate to succeed (e.g. require.NoError(t, db.AutoMigrate(&TestModel{})), failing the test immediately on error), and move DropTable cleanup into t.Cleanup with its error asserted.
- In the
BeforeUpdate test, don't pre-set UpdatedAt. Change only a non-timestamp field, save, reload, and assert the reloaded UpdatedAt differs from the originally-captured originalUpdatedAt, so the assertion actually depends on the hook running.
Version
Surfaced on PR #517 (refactor/portal-backend-restructure).
Additional Context
Flagged 🟠 Major (discarded errors) and 🟡 Minor (weak assertion) by CodeRabbit. Similar in spirit to #503 (portal_handler_test.go: tests don't exercise the branches they claim to) and #513 (OE stale docs and weak test assertions).
Describe the Bug
Two issues in
internal/pb/v1/models/base_test.go, surfaced by CodeRabbit reviewing #517 — both predate that PR, they just moved location (wasportal-backend/v1/models/base_test.go).Discarded errors (lines ~29-30 and ~63-64): both
TestBaseModel_BeforeCreateandTestBaseModel_BeforeUpdatecalldb.AutoMigrate(&TestModel{})anddefer db.Migrator().DropTable(&TestModel{})without checking their errors. This is flagged byerrcheckand blocks the lint gate; a failed migration can also produce a confusing downstreamCreate/Savefailure instead of a clear migration error.Self-fulfilling assertion (lines ~76-89):
TestBaseModel_BeforeUpdate's subtest setsmodel.UpdatedAt = explicitLaterTimedirectly before callingdb.Save(&model), then asserts the reloaded value equals (or is after) that same explicitly-set value. Since the test pre-sets the exact field theBeforeUpdatehook is supposed to set, the assertion passes whether or not the hook actually runs — it doesn't verify the hook's lifecycle behavior at all.Expected Behavior
AutoMigrateto succeed (e.g.require.NoError(t, db.AutoMigrate(&TestModel{})), failing the test immediately on error), and moveDropTablecleanup intot.Cleanupwith its error asserted.BeforeUpdatetest, don't pre-setUpdatedAt. Change only a non-timestamp field, save, reload, and assert the reloadedUpdatedAtdiffers from the originally-capturedoriginalUpdatedAt, so the assertion actually depends on the hook running.Version
Surfaced on PR #517 (
refactor/portal-backend-restructure).Additional Context
Flagged 🟠 Major (discarded errors) and 🟡 Minor (weak assertion) by CodeRabbit. Similar in spirit to #503 (
portal_handler_test.go: tests don't exercise the branches they claim to) and #513 (OE stale docs and weak test assertions).