I strongly agree with Gian about underscores being ugly, though I think your suggested formatting would be just as readable (to me at least) using just camel-case and would also remain consistent with the styling of the majority of Druids existing tests. I share similar concerns with the others on how well it holds up as a convention beyond simple unit tests. I also don't really feel that the utility this naming adds is worth treating it as a hard rule, at least at this point. In my experience, the most useful thing in determining why a test has failed and what my code did to do it is generally either the stack trace of the error or the mismatched values in the test expectation. The test name doesn't really matter to me at least, and is more of just an anchor point to navigate to a position in a file to let me know which test(s) to re-run, where as the other information is usually all it takes to clue me into what I part of the changes in my PR likely caused the test to fail.
Because of this, and more importantly that there will be no possible way that tooling can enforce such a naming rule, I think it instead should just be captured as best practices in CONTRIBUTING.md or something like that, as just a suggestion, so that we don't have to nag at people to enforce this. On Mon, Dec 23, 2019 at 7:15 PM Jonathan Wei <jon...@apache.org> wrote: > I think the suggested format could be useful when applicable, I would > consider expanding "function under test" to "area under test" to > accommodate tests that have a wider scope than a single method. > > For cases where that format may not be applicable, perhaps that means it's > a test where additional javadocs/comments explaining what the test > does/expects is a good idea (that would be enough info in my view). > > Since I don't think the proposed format applies universally, I would prefer > starting it off as a suggestion/best practice instead of as a hard > requirement and seeing how that goes. > > I'm indifferent to the use of underscores. > > > > On Mon, Dec 23, 2019 at 3:19 PM Gian Merlino <g...@apache.org> wrote: > > > Suneet, > > > > Sometimes it's hard to understand how things would improve without an > > example. Could you point to a test file that you think would be improved > by > > this change? Also, there are some test files that I would struggle to fit > > into this framework. It seems best suited to simple single-method unit > > tests. What would you suggest doing for these examples? > > > > - CalciteQueryTest: Tests the SQL layer. There's one function per SQL > query > > being tested, but there is no specific function under test, and the > > expected result is too complex to include in a method name. > > - GroupByQueryRunnerTest: Tests the groupBy query engine. Similar > structure > > and similar issue to CalciteQueryTest. > > - SelectorFilterTest: Tests the selector filter. But no specific function > > is being tested. The "assertFilterMatches" helper is used to create test > > cases that say 'this filter should match these rows'. > > > > Other notes: > > > > - IMO, the underscores are ugly, but acceptable if they buy us something. > > - I'm not sure that the "expectedResult" part of the name is going to > work > > well. It's often something complex (like an array, or object, or long > > string) that would be awkward to include in a method name. And with JUnit > > assertions, the expected result is right there in the method call anyway. > > > > Looking forward to hearing your thoughts. Thanks for thinking about how > to > > make our testing better. > > > > Gian > > > > On Mon, Dec 23, 2019 at 11:06 AM Suneet Saldanha < > suneet.salda...@imply.io > > > > > wrote: > > > > > Hello, > > > > > > I've started writing tests in the druid repo and would like to propose > a > > > new test naming structure for the project. Inspired by this thread - > > > https://www.mail-archive.com/dev@druid.apache.org/msg02426.html but > > > focused > > > only on the naming of tests. I'd like to propose we start using the > > format > > > below > > > > > > test_<functionUnderTest>_<conditions>_<expectedResult> > > > > > > This makes it a lot easier for devs to name tests in a way that's > easily > > > understood by someone else without having to read the test to know > what's > > > going on. Here's some of my rationale: > > > > > > - Explicit - the name tells you everything you need to know about > the > > > test > > > - Forces you to write one test per condition/ expected result > > > - Underscores make it easy to delineate the different components of > > the > > > test > > > - Minimal effort to think of a short name that correctly captures > > > everything about the test while still being different from all the > > other > > > tests > > > > > > Happy to hear feedback / concerns about this approach. > > > > > > Suneet > > > > > >
