jihoonson commented on pull request #11822: URL: https://github.com/apache/druid/pull/11822#issuecomment-981904755
> It seems we've got a situation where we can't clean up warnings in code until we figure out how to fully unit test the code. While having tests is a Good Thing, it seems we've wrapped ourselves around the axel because we didn't require tests when the code was written, but we do require someone else to write the tests to clean up warnings. @paul-rogers thanks for bringing this up. People, including myself, have been struggling with a similar issue from time to time. The coverage bot is good to have because it reduces the burden of code review. The reviewer should manually check test coverage otherwise. For your point, I don't think we can enforce the original author to add all tests that cover 100% of the change because it will be another way to wrap ourselves around the axel :slightly_smiling_face: Making a PR will become extremely hard if we raise the bar of coverage check to 100%. I think our current problem is that the bot is not smart enough to tell what are real changes that impact production code path and what are not. We should work on this to make the bot better over time. For this particular PR, I think there could be 3 options for us. - Not fixing the warning (or just suppressing the warning). The warning seems trivial and I don't mind living with it. - Ignoring the test coverage check failure. This makes sense only when all tests in the phase 2 pass. Travis allows us to manually start those tests, but they should be started one by one by clicking the restart button in Travis. This is doable but there are more than 60 tests in the phase 2 and my wrist will hurt after restarting all of them :slightly_smiling_face: - Adding a new test. I prefer this option most as it's good to have a larger test coverage. It seems not that hard to add a test for this issue if you add a custom fjp pool that always explodes in `execute()` since it is always called in `MergeCombinePartitioningAction.compute()`. You may want to make `MergeCombinePartitioningAction` `VisibleForTesting` for easy testing. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
