Skip to content

test: improve SearchOperationsTest postgres coverage#3340

Open
msmithstubbs wants to merge 3 commits intoLogflare:mainfrom
msmithstubbs:test/search-ops-postgres
Open

test: improve SearchOperationsTest postgres coverage#3340
msmithstubbs wants to merge 3 commits intoLogflare:mainfrom
msmithstubbs:test/search-ops-postgres

Conversation

@msmithstubbs
Copy link
Copy Markdown
Contributor

Implement feedback on test improvements from #3307

  • use `TestUtils.setup_single_tenant' macro
  • remove unnecessary mocks
  • stub exception for postgres error

@msmithstubbs msmithstubbs force-pushed the test/search-ops-postgres branch from cfe64e4 to 8a6963a Compare April 3, 2026 02:28
@msmithstubbs msmithstubbs marked this pull request as ready for review April 3, 2026 02:43
Copy link
Copy Markdown
Contributor

@amokan amokan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @msmithstubbs 👋

The test changes look good. My only concern is the addition of the rescue block in search operations - which does change production behavior a bit.

If there is a need to include the rescue block, could we at least tighten that up to only rescue Postgrex.Error?

@msmithstubbs msmithstubbs force-pushed the test/search-ops-postgres branch from c404024 to f13bfe7 Compare April 9, 2026 04:50
@msmithstubbs
Copy link
Copy Markdown
Contributor Author

The test changes look good. My only concern is the addition of the rescue block in search operations - which does change production behavior a bit.

If there is a need to include the rescue block, could we at least tighten that up to only rescue Postgrex.Error?

Yeah, you're right: we don't need it. I've removed it and updated the test to assert any exceptions are passed through.

Might be worth considering rescuing some exceptions to align the Postgres code path closer to BigQuery but that should be a separate PR.

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