> 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. > > Benjamin Hindman wrote: > 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.
as discussed offline, my online concern was the name of the class. Perhaps we can call it TestFilter? > 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? > > Benjamin Hindman wrote: > 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? IIUC, a failing ASSERT_* would still call the TearDown()/Destructor of the fixture. So, how is this a problem? I agree, though, that stuffing it inside the destructor is a good idea and less likely be abused by test writers. - Vinod ----------------------------------------------------------- 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 > >
