-
Notifications
You must be signed in to change notification settings - Fork 58
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
[Enhancement] Improve test coverage for SDK #703
Comments
OK, time to get on my soapbox again. :-) First, note that I asked about this in #250 and got no responses other than my own, which became the de facto default. I'll rehash some of my opinions from that issue. TLDR, I think code coverage is an arbitrary metric that's easy to cheat. It can give you the illusion of well tested software when in fact it's not. I like code coverage reports that we can drill into periodically in order to find things we may have missed (as you have done with this issue). I dislike it as a mandatory element of PR workflow that encourages devs to find the fastest way to get a passing metric, even if it's a useless test. I can easily "cover" a line of code by executing it. But am I properly testing the assertions? The API link above is mostly copy/paste from the plugin interfaces... do we really need to test that a default empty list is an empty list? Or any other default value? What value does that bring? Other than the default returns, the offending code appears to be The SDKTransportService code is a good side-by-side example. The ActionListener I don't think we can even test outside of IT. The handler code we could probably test more rigorously with unit tests, but it's best tested in an IT environment as well. OK, back off my soapbox. My thoughts:
|
Whoever does |
Thanks @dbwiddis for writing this up. I've added discuss label. This issue is a consequence of #704, we were trying to |
Hi @saratvemulapalli, yes I definitely plan to help on this issue and I'll try to highlight out some of the challenges to get better coverage in this area. For context, the security plugin performs a lot of the testing of SSL configuration in this test class: SSLTest. This test class subclasses SingleClusterTest which subclasses AbstractSecurityUnitTest. The tests in this suite also utilize the ClusterHelper to spin up in memory clusters with different configurations. I highlighted the areas of the security codebase above to try to give examples of how I think the test framework for testing changes like SSL Configuration should be. I'd like to create the ability to create in-memory configurations of an extension + an opensearch node (with security enabled) and test out the different configuration options for SSL and verify that extension bootstrap happens successfully. I briefly took a look on Tuesday to see what would be required to build this kind of framework and the process wasn't straightforward. |
I will look into whether its possible to publish a test jar from the security plugin and utilize that for tests in this repo. |
@cwperks @saratvemulapalli given #714 we may not want to spend too much time figuring out a solution in this repo. I think we all agree the code needs to be tested, but the new location would be a better place to do that. We can configure jacoco to skip that package. |
Is your feature request related to a problem?
Code coverage for SDK is trending downwards and is at 66% on 25th April 2023[1].
Writing up this issue to improve our coverage to atleast 75%.
Feel free to pick up any of these to improve test coverage
Proposal
[1] https://app.codecov.io/gh/opensearch-project/opensearch-sdk-java/tree/main/src/main/java/org/opensearch/sdk
The text was updated successfully, but these errors were encountered: