+1 agreement with Kirk and Sean. Any non default configuration should probably have it's own set of tests. I can understand some exploratory work where someone might want to run the whole precheckin with a non default value to help identify areas that they may have missed or are unexpectedly affected. At that point the relevant tests should have been identified and copies of them could be made with the new configuration.
On Thu, Mar 15, 2018 at 9:46 AM Sean Goller <sgol...@pivotal.io> wrote: > I agree with this. We should have a default state that reflects an “out of > the box” configuration, and if tests expects a different configuration, it > should manage that within the context of the test. > > -Sean > > On Tue, Mar 13, 2018 at 10:04 AM Kirk Lund <kl...@apache.org> wrote: > > > I want to propose using the default value for > validate-serializable-object > > in dunit tests instead of forcing it on for all dunit tests. I'm > > sympathetic to the reason why this was done: ensure that all existing > code > > and future code will function properly with this feature enabled. > > Unfortunately running all dunit tests with it turned on is not a good way > > to achieve this. > > > > Here are my reasons: > > > > 1) All tests should start out with the same defaults that Users have. If > we > > don't do this, we are going to goof up sometime and release something > that > > only works with this feature turned on or worsen the User experience of > > Geode in some way. > > > > 2) All tests should have sovereign control over their own configuration. > We > > should strive towards being able to look at a test and determine its > config > > at a glance without having to dig through the framework or other classes > > for hidden configuration. We continue to improve dunit in this area but I > > think adding to the problem is going in the wrong direction. > > > > 3) It sets a bad precedent. Do we follow this approach once or keep > adding > > additional non-default features when we need to test them too? Next one > is > > GEODE-4769 "Serialize region entry before putting in local cache" which > > will be disabled by default in the next Geode release and yet by turning > it > > on by default for all of precheckin we were able to find lots of problems > > in both the product code and test code. > > > > 4) This is already starting to cause confusion for developers thinking > its > > actually a product default or expecting it to be enabled in other > > (non-dunit) tests. > > > > Alternatives for test coverage: > > > > There really are no reasonable shortcuts for end-to-end test coverage of > > any feature. We need to write new tests or identify existing tests to > > subclass with the feature enabled. > > > > 1) Subclass specific tests to turn validate-serializable-object on for > that > > test case. Examples of this include a) dunit tests that execute Region > > tests with OffHeap enabled, b) dunit tests that flip on HTTP over GFSH, > c) > > dunit tests that run with SSL or additional security enabled. > > > > 2) Write new tests that cover all features with > > validate-serializable-object > > enabled. > > > > 3) Rerun all of dunit with and without the option. This doesn't sound > very > > reasonable to me, but it's the closest to what we really want or need. > > > > Any other ideas or suggestions other than forcing all dunit tests to run > > with a non-default value? > > > > Thanks, > > Kirk > > >
