> 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. > > Benno Evers wrote: > 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?
Anyways, I'm going to discard this until I get a better idea for validation. - 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 > >
