Move error_test.go to a separate test package #5796
Conversation
…t to a test package that are not ignored
jakobht
requested review from
Shaddoll,
neil-xie,
davidporter-id-au,
Groxx,
shijiesheng,
agautam478,
3vilhamster,
sankari165,
dkrotx,
taylanisikdemir and
demirkayaender
as code owners
Codecov Report
Additional details and impacted filessee 4 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
| @@ -20,17 +20,18 @@ | |||
| // OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE | |||
| // SOFTWARE. | |||
|
|
|||
| package testdata | |||
| package testdata_test | |||
There was a problem hiding this comment.
FYI currently the coverage numbers we collect only include the tests targeting the code in the same package. So a separate package with unit tests don't add to coverage numbers. There are workarounds for this setup but it increases total test time.
Looks like this file is only covering functionality of some stuff in testdata which is not part of actual code so maybe we can leave this there
There was a problem hiding this comment.
Oh, that's sad, I was not aware.
Go has some magic folder names (e.g. starting with _ and testdata) for which it does not run tests at all, so that's why I moved it out. So now the test is acctually run by the CI (before it was not).
There was a problem hiding this comment.
@taylanisikdemir I'd broadcast this to the team, I wasn't aware either and this convention is pretty common
There was a problem hiding this comment.
I agree, just still note that in this PR, we have to move it out as tests in folders called testdata are never picked up by go test.
Another option is to rename the testdata folder, but then we have to write tests for all the files in testdata, currently these files are (hopefully) not counted in the coverage numbers.
There was a problem hiding this comment.
Stuff in testdata should only be used by other tests and shouldn't require to be tested. If this functionality deserves its own tests then let's move it to some other folder or add those tests to one of the relevant _test.go file. No need to track coverage for this.
There was a problem hiding this comment.
Moved to the testing package
Also adds the allIsSet function as a utility folder
Pull Request Test Coverage Report for Build 018ebc69-3ae6-44df-a367-9b0fc40d6c69Details
💛 - Coveralls |
common/testing/allIsSet_test.go
Outdated
| @@ -0,0 +1,40 @@ | |||
| // The MIT License (MIT) | |||
There was a problem hiding this comment.
We have been renaming files to snake_case as we touch them
What changed?
Moved error_test.go to a separate test package
Why?
The folder testdata is ignored when running tests, so moving this test to a test package that isn't ignored
How did you test it?
Made sure the test runs when doing
go testandmake testPotential risks
Release notes
Documentation Changes