mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benno Evers <bev...@mesosphere.com>
Subject Re: Review Request 69584: Relaxed matching criteria for test filters.
Date Wed, 19 Dec 2018 22:00:59 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message