> On Oct. 24, 2012, 10:32 p.m., Vinod Kone wrote: > > src/tests/allocator_tests.cpp, line 571 > > <https://reviews.apache.org/r/7719/diff/1/?file=179398#file179398line571> > > > > Please revert. The former is def clearer and tells me that this is a > > test/mock filter.
But I don't want people to be using the filter like a "mock". We aren't _testing_ the filter, we are _using_ the filter. The abstractions one should use are EXPECT_MESSAGE (and maybe in the future EXPECT_HTTP, or EXPECT_DISPATCH). The implementation happens to use gmock, but that's just an implementation detail (and might in fact change in the future). I would have preferred to have a test fixture called FilteringTest that didn't require even a programmer to write 'Filter filter;' at the beginning of every test but just let them use EXPECT_MESSAGE directly, but that didn't seem to work well with non fixture based tests like TYPED_TEST. Thus the compromise was a Filter implementation in the 'tests' namespace, which seemed pretty clear to me that it is used for testing. > On Oct. 24, 2012, 10:32 p.m., Vinod Kone wrote: > > src/tests/gc_tests.cpp, line 169 > > <https://reviews.apache.org/r/7719/diff/1/?file=179402#file179402line169> > > > > why was this removed from the fixture? As the description of this review says, we want the filter to always be removed after a test finishes. The problem was that an ASSERT_* might get invoked in a test which would abort the test before invoking process::filter(NULL), thus a future test might actually try and call into that filter which was on the stack and now obliterated (causing a SIGSEGV). Using a fixture here is the perfect solution (as I mention above). However, it wasn't easy to make every test that used a filter to be able to extend from a FilteringTest fixture. So, I wanted to make sure that any test which uses a Filter makes sure that process::filter(NULL) gets called. I solved this by putting process::filter(NULL) in the destructor. Furthermore, when you reuse a filter you have to make sure that you've cleared any expectations that had already been set (which again was breaking the implementation/interface barrier). Thus, it seemed easier to rely on the simple semantics of a new Filter instance each time than needing to explicitly "clear" a filter in SetUp (which I wasn't always sure had the correct semantics) and do process::filter(NULL) in TearDown. I'm still not convinced this is perfect (because someone can blatantly use Filter in a fixture without doing the explicit stuff I mentioned above). One thought was to have a global filter and use test listeners to properly "clear" a filter after every test and let people just use EXPECT_MESSAGE whenever they want without even having to declare 'Filter filter;'. I just decided this was for another day. Other suggestions? - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/7719/#review12731 ----------------------------------------------------------- On Oct. 24, 2012, 4:57 a.m., Benjamin Hindman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/7719/ > ----------------------------------------------------------- > > (Updated Oct. 24, 2012, 4:57 a.m.) > > > Review request for mesos, Vinod Kone and Ben Mahler. > > > Description > ------- > > See summary. > > > Diffs > ----- > > src/java/jni/org_apache_mesos_Log.cpp > 4f27d3ff76c172ab1d3ff6b804bb65e9256ee637 > src/log/coordinator.hpp 01905da7558b1ec8f6d50c5e381dc0ec25c69cc9 > src/log/coordinator.cpp 134e655c6211eba189375ed9697d31f5d60decd0 > src/log/log.hpp ac7aad2dac646a726d4cdecae86c949c9865d917 > src/tests/allocator_tests.cpp ed52ff61b1820cb3f5f7f2ae421fb3f84a85939a > src/tests/allocator_zookeeper_tests.cpp > bd408c22df5edc3684791e509ce1a4c210553922 > src/tests/exception_tests.cpp f99c92045c68676991fec61f7613c804eaffaad6 > src/tests/fault_tolerance_tests.cpp > 558bf41ab6f4ba319ec3b23f3347f98e235260f9 > src/tests/gc_tests.cpp 1de240579a1cc6af8738d2ffb5da1b3e01c4b19e > src/tests/log_tests.cpp 26aa9dda5e95f3235fff83970978fe79247ada3e > src/tests/master_tests.cpp 37e97e0a58a0128579f155183b7e769ab4e1b452 > src/tests/utils.hpp 4066c02b23db15310104955465c4e4afd7bcad5b > > Diff: https://reviews.apache.org/r/7719/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Benjamin Hindman > >
