That's a great link, thank you! What do you think we should change in the process to start following these patterns? Should the committers be more strict on what gets committed?
On Fri, Oct 4, 2019 at 9:44 AM Roman Leventov <leven...@apache.org> wrote: > A few days ago I've stumbled upon a blog post "Modern Best Practices for > Testing in Java" ( > https://phauer.com/2019/modern-best-practices-testing-java/) and learned a > lot of new things from it. > > In a lot of cases, how testing is done in Druid goes against these best > practices: > - "Given, When, Then" structure: in Druid, rarely followed or clarified > via blank lines. > - Use the Prefixes “actual*” and “expected*: not in Druid, usually it's > something1 / something2 and what is expected and what is actual is unclear > (and sometimes confused in assert() args) > - Heavily Use Helper Functions: used sometimes in Druid, but by far not > enough, in my perception > - Don’t Extend Existing Tests To “Just Test One More Tiny Thing: not > everywhere, but in a lot of places this practice is abused in Druid tests > - Insert Test Data Right In The Test Method: test data is often inserted > in @Before in Druid > - Favor Composition Over Inheritance: there are some hierarchies in Druid > - Don’t Write Too Much Logic: Druid tests feel like *all* > boilerplate/noise/scaffolding logic > - Don’t Use In-Memory Databases For Tests: Druid does toy database Derby > for tests :) > - Use AssertJ: Druid doesn't use AssertJ (I've just added AssertJ to > dependencies in https://github.com/apache/incubator-druid/pull/8564 in one > module) > - Use JUnit5: ¯\_(ツ)_/¯ > - Mock Remote Service: in Druid, we don't use OkHttp's MockWebServer > - Use Awaitility for Asserting Asynchronous Code: in Druid, we use ad-hoc > while loops with sleep(10), sleep(100), etc. > - Separate Asynchronous Execution and Actual Logic: I think, this is one > of the most important points. In Druid, in almost all cases, execution > management (thread pools), dependency injection (and dependencies in > general), and the "core" logic are alloyed in a single class. This makes > testing the core logic very painful with a lot of mocking and setup > ceremony, breaking frequently. Following the Humble Object pattern ( > http://xunitpatterns.com/Humble%20Object.html) would make writing *and > supporting* much less laborious. > > These things cannot be fixed in one day because test code has a lot of > inertia, but I believe to improve developer productivity all active Druid > developers should start using most of these practices in all new tests > being written, demand other contributors align their tests with these > practices in code reviews, and take some time to think about strategic ways > to refactor a bulk of existing tests with little effort > (semi-automatically) that can at the same time improve their quality even > by a little. > > In my experience, wrestling with tests is a huge (often the biggest) part > of changing existing code in Druid, so we pay a huge productivity tax by > not paying enough attention to the quality of the test codebase. > -- Jad Naous Imply | VP R&D 650-521-3425 jad.na...@imply.io @jadtnaous <https://twitter.com/jadtnaous>
