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

Update CONTRIBUTING to remove section on internal syncs #865

Merged
merged 1 commit into from
Jun 8, 2023

Conversation

wenovus
Copy link
Collaborator

@wenovus wenovus commented Jun 8, 2023

It looks like this section is outdated.

Thanks @lgomez9 for spotting.

It looks like this section is outdated.

Thanks @lgomez9 for spotting.
@wenovus wenovus requested a review from robshakir June 8, 2023 19:35
@coveralls
Copy link

Coverage Status

coverage: 89.749%. remained the same when pulling 4547fc8 on update-contributing into 85d7eaf on master.

@robshakir
Copy link
Contributor

robshakir commented Jun 8, 2023

our coverage fell below 90%, makes me sad! should we have a fixit for addressing this at some point? :-)

@wenovus
Copy link
Collaborator Author

wenovus commented Jun 8, 2023

our coverage fell below 90%, makes me sad! should we have a fixit for addressing this at some point? :-)

Sure, I can take a look at increasing the coverage for the internal/yreflect package to hit more error handling lines. Although I think 90% is just around the point of diminishing returns given how common error handling lines occur in Go. I think it's important to hit error handling lines for the originator of errors but it's less important for downstream code that catch these errors. We would not be able to be much above 90% unless we write tests for those cases as well.

@wenovus wenovus merged commit 6fe7ca0 into master Jun 8, 2023
@wenovus wenovus deleted the update-contributing branch June 8, 2023 20:45
@wenovus
Copy link
Collaborator Author

wenovus commented Jun 8, 2023

our coverage fell below 90%, makes me sad! should we have a fixit for addressing this at some point? :-)

Sure, I can take a look at increasing the coverage for the internal/yreflect package to hit more error handling lines. Although I think 90% is just around the point of diminishing returns given how common error handling lines occur in Go. I think it's important to hit error handling lines for the originator of errors but it's less important for downstream code that catch these errors. We would not be able to be much above 90% unless we write tests for those cases as well.

@robshakir or @hellt Would be happy to hear your opinions as well on coverage for error handling. Usually when I write tests I focus on whether I'm covering the features and corner cases instead of coverage, and I simply use coverage to see where I missed important test cases. I get disappointed when it's simply error catching and return statements that reduce headline coverage and now I've mostly given up on conjuring test cases that exercise these code paths.

Here is a blog I found on this topic:
https://testing.googleblog.com/2020/08/code-coverage-best-practices.html

Make sure that frequently changing code is covered. While project wide goals above 90% are most likely not worth it, per-commit coverage goals of 99% are reasonable, and 90% is a good lower threshold. We need to ensure that our tests are not getting worse over time.

per-commit coverage goals of 99% are reasonable -- this doesn't sound realistic to be for Go code.

A high code coverage percentage does not guarantee high quality in the test coverage.
But a low code coverage number does guarantee that large areas of the product are going completely untested

So I agree with the above two statements, but I wonder what the right "low code coverage" is for Go. I remember going through the exercise of trying to cover these error statements before but I started to feel it was not worth it quickly after I started. Would be curious to see what your thoughts on this topic are.

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.

3 participants