Nothing in particular. I think just reading the article. And also actually paying attention to test quality. In the past, on many occasions, I've skipped reading the test code in pull-requests completely or glanced over it very cursory which is a bad practice.
The questions that developers and reviewers should ask themselves are - Will it be easy to add more tests to this class? - Will it be easy to support existing and new tests? - Will the test break (or would require rework) when somebody changes implementation internals, e. g. due to mocking not working properly? On Fri, 4 Oct 2019 at 21:57, Jad Naous <jad.na...@imply.io> wrote: > 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> >
