Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add IgnoreErrorFn to slogtest options #217

Merged
merged 1 commit into from
Nov 12, 2024
Merged

Conversation

spikecurtis
Copy link
Contributor

This PR introduces a new option to slogtest: IgnoreErrorFn, which allows test code to selectively ignore errors based on the contents of the SinkEntry.

primary use case:

We're still fighting flakes where the error is due to canceling the context on shut down, but when canceling active postgreSQL queries, we can get errors like: pq: canceling statement due to user request.

e.g. https://github.com/coder/coder/actions/runs/11681605747/job/32527006174

    t.go:108: 2024-11-05 09:52:37.996 [erro]  workspacestats: failed to load template schedule bumping activity, defaulting to bumping by 60min  request_id=f14215d2-73dc-47ba-aa81-422c62f257e4  workspace_id=545d73c7-3a62-4466-8c08-b6abb12867b7  template_id=49747428-3abb-40e4-a6b2-03653e9f2506 ...
        error= fetch object:
                   github.com/coder/coder/v2/coderd/database/dbauthz.(*querier).GetTemplateByID.fetch[...].func1
                       /home/runner/work/coder/coder/coderd/database/dbauthz/dbauthz.go:497
                 - pq: canceling statement due to user request
         *** slogtest: log detected at level ERROR; TEST FAILURE ***

Unfortunately, we can't use IgnoredErrorIs for this, because the exported pq.Error type encompasses all PostgreSQL errors, not just query canceled. But, if we have the freedom to use a function to ignore errors, we can use database.IsQueryCanceledError to weed out these flakes.

secondary use case:

I decided to pass the entire SinkEntry to the function, so that tests can be really targeted about which errors to ignore, if they want to. They could match on the error message, or even other fields so that you can ignore an error for a request that is expected to fail, but not mask failures for other similar requests that are supposed to succeed.

@ammario ammario removed their request for review November 6, 2024 16:56
@spikecurtis spikecurtis force-pushed the spike/ignore-error-fn branch from 3c57153 to 6f99a7c Compare November 8, 2024 06:15
Copy link

github-actions bot commented Nov 8, 2024

Pull Request Test Coverage Report for Build c81cb4e378f17fe114d95a637ead89e678c6bbe9-PR-217

Details

  • 14 of 14 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.03%) to 97.044%

Totals Coverage Status
Change from base Build 3e5cea5e4d6a8c8396f626a8a4989cc02aea4ed1: 0.03%
Covered Lines: 788
Relevant Lines: 812

💛 - Coveralls

@spikecurtis spikecurtis merged commit 0ec81e6 into main Nov 12, 2024
1 check passed
@spikecurtis spikecurtis deleted the spike/ignore-error-fn branch November 12, 2024 04:18
spikecurtis added a commit to coder/coder that referenced this pull request Nov 18, 2024
Refactors our use of `slogtest` to instantiate a "standard logger" across most of our tests.  This standard logger incorporates coder/slog#217 to also ignore database query canceled errors by default, which are a source of low-severity flakes.

Any test that has set non-default `slogtest.Options` is left alone. In particular, `coderdtest` defaults to ignoring all errors. We might consider revisiting that decision now that we have better tools to target the really common flaky Error logs on shutdown.
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