Skip to content

Conversation

@spikecurtis
Copy link
Contributor

I feel like we spend a lot of time chasing flakes due to logging context.Canceled at Error in product code, because a lot of our tests shut down by canceling contexts. This is not a good use of our time, so we should, by default, ignore these errors vis a vis erroring the test case in slogtest

Signed-off-by: Spike Curtis <spike@coder.com>
@github-actions
Copy link

github-actions bot commented Jan 25, 2024

Pull Request Test Coverage Report for Build 051f52451600cfdd55b8c1c1cc95f2abf43965ff-PR-207

  • 0 of 17 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.3%) to 97.015%

Totals Coverage Status
Change from base Build f0c466fabe10641afdbcc629938df29f941b3d18: 0.3%
Covered Lines: 780
Relevant Lines: 804

💛 - Coveralls

// conditions exist when t.Log is called concurrently of a test exiting. Set
// to true if you don't need this behavior.
SkipCleanup bool
// IgnoredErrorValues causes the test logger not to error the test on Error
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// IgnoredErrorValues causes the test logger not to error the test on Error
// IgnoredErrorIs causes the test logger not to error the test on Error

Comment on lines 57 to 60
opts.IgnoredErrorIs = []error{
context.Canceled,
context.DeadlineExceeded,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be nice to include this as an exported slice or have some function that produces an IgnoredErrorIs slice with these included, so you don't have to manually specify them every time you want to add an extra error instead of overriding them.

Signed-off-by: Spike Curtis <spike@coder.com>
@spikecurtis spikecurtis merged commit 20367d4 into main Jan 26, 2024
@spikecurtis spikecurtis deleted the spike/ignore-context-canceled branch January 26, 2024 06:47
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