Skip to content

Conversation

EldarKurbanov
Copy link
Contributor

@EldarKurbanov EldarKurbanov commented Dec 6, 2021

Closes #58

I also added a goroutine leak detector to prevent unclosed connections in the future and one exception for goroutine leak detector, since I did not find how to fix it.

@EldarKurbanov
Copy link
Contributor Author

CI didn't like my go.sum. How can I fix it?

@fergusstrange
Copy link
Owner

Hey @EldarKurbanov you'll need to run go mod tidy in the top level module and each of sub modules to clean up the go.sum files.

Regarding the test coverage changes I'd imagine you probably only need to verify no leak in one of the top level tests as they cover almost all the code and run on every operating system.

@EldarKurbanov
Copy link
Contributor Author

EldarKurbanov commented Dec 10, 2021

Could you help me a bit and clarify a few things: if I understood correctly, I need to leave a check for a goroutine leak only in one test?

@fergusstrange
Copy link
Owner

Hi @EldarKurbanov apologies for the delay.

Yes I suggest this is probably only actually needed inline inside the Test_DefaultConfig test as it should cover most lines of code already.

We also should attempt to keep the coverage above 90% as a general rule project so users can be confident from a glance we're reliable. Is there anything you can do to get those extra lines you've added covered?

@EldarKurbanov
Copy link
Contributor Author

I would really be very happy if you could tell me how I can check my added code that close the connection to the database.

The only one the way I see for increase test coverage is by adding a mock to the database connection. I can do it if there are no better ideas and no one will mind.

@EldarKurbanov
Copy link
Contributor Author

I removed defer verifyLeak(t) as you suggested and left it only in Test_DefaultConfig.

@fergusstrange
Copy link
Owner

fergusstrange commented Jan 15, 2022

Looks much better! I think you're correct maybe wrapping db := sql.OpenDB in a func and passing that into these methods, or utilising a new interface that wraps up these responsibilities would be the best way forward.

There are some good examples of making sure these error cases are covered in decompression_test.go if you need a template to work off. This is largely utilising functional wrappers, the preferred approach in this library.

@EldarKurbanov
Copy link
Contributor Author

Thank you for your suggestion! But I found an easier way to test the database connection closing code, wrote a test for it, and the coverage increased. I would be very glad to hear your opinion about the rewritten code.

@fergusstrange fergusstrange merged commit 6c3027c into fergusstrange:master Feb 6, 2022
@fergusstrange
Copy link
Owner

Hey @EldarKurbanov sorry this has taken so long! I needed some bandwidth to get my head around your changes. They make sense to me now. I may try and clean up a little in the next few releases to align better with the style of the rest of the code, and make the tests a little more verbose, but functionally this looks great.

Thanks for your contribution and hopefully this helps resolve your issues!

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.

Goroutine leak on database connect
2 participants