> On Dec. 18, 2018, 6:44 p.m., Meng Zhu wrote: > > Not quite sure about the false positive issue: > > > > "This change is also safe regarding false positives, since our > > naming conventions forbid the matched strings from appearing > > naturally in any test name." > > > > Ah, I wasn't aware of the convention. But I feel it is easy to slip? > > For example, after this patch, would we match > > "ROOT_DOCKER_CGROUPS_CFS_CgroupsEnableCFS" if I remove the "CFS_" tag? > > > > If that is the case, I would prefer to keep the original matching criteria > > and fix the test name. It is easy to > > spot a test that ran (but not supposed to) than finding out a silent > > missing test. > > Joseph Wu wrote: > I agree about the false positive case. We should just update the > specific BENCHMARK test to properly filter it. > > I don't know if we codify our test naming style anywhere at the moment. > However, our naming convention is to put any of the test filters at the > beginning of a test name, rather than the end.
The style guide rule I had in mind when writing that paragraph of the commit messages was this: > For some symbols, this style guide recommends names to start with a capital > letter and to have a capital letter for each new word (a.k.a. "Camel Case" or > "Pascal case"). When abbreviations or acronyms appear in such names, prefer > to capitalize the abbreviations or acronyms as single words (i.e StartRpc(), > not StartRPC()). I agree that the test case name is not correct according to our naming convention, and I'd be happy to give a 'Ship It' to a review that puts the `BENCHMARK_` in front of the name. Similarly, I'd also argue that `ROOT_DOCKER_CGROUPS_CFS_CgroupsEnableCFS` is incorrectly named and should be renamed to `ROOT_DOCKER_CGROUPS_CFS_CgroupsEnableCfs`. (note that in the former case, it will be easy to miss that e.g. a review introducing a new test `CgroupsEnableCFS_WithoutRoot` accidentally matches the filter) However, if we do just that then we're back in exactly the same situation that we were in before this was originally committed, and it seems like just a matter of time until someone else is running into the same trap. Maybe it would be possible to add some validation for test names instead, either as a linter rule or a runtime check? - Benno ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69584/#review211410 ----------------------------------------------------------- On Dec. 18, 2018, 2:59 p.m., Benno Evers wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69584/ > ----------------------------------------------------------- > > (Updated Dec. 18, 2018, 2:59 p.m.) > > > Review request for mesos and Meng Zhu. > > > Repository: mesos > > > Description > ------- > > Changed the test filters to remove the required trailing > underscore, e.g. the `BenchmarkFilter` would now match all test names > containing the string `BENCHMARK`, as opposed to `BENCHMARK_`. > > The latter filter would fail to match test names like > `HierarchicalAllocations_BENCHMARK.MultiFrameworkAllocations`, > contrary to the test authors assumptions. > > This change is also safe regarding false positives, since our > naming conventions forbid the matched strings from appearing > naturally in any test name. > > > Diffs > ----- > > src/tests/environment.cpp 03f8f5971b1e91e62ecfbf965028e6f2388d2a7b > > > Diff: https://reviews.apache.org/r/69584/diff/2/ > > > Testing > ------- > > > Thanks, > > Benno Evers > >
