<bump> I filed GEODE-6202: DUnit should not enable VALIDATE_SERIALIZABLE_OBJECTS by default https://issues.apache.org/jira/browse/GEODE-6202
And submitted PR #3023 https://github.com/apache/geode/pull/3023 Please review and/or discuss further if needed. Thanks, Kirk On Thu, Mar 15, 2018 at 12:00 PM Bruce Schuchardt <bschucha...@pivotal.io> wrote: > +0.5 I think we can turn this off (back to the default) now since the > AnalyzeSerializables tests for other modules have been created. These > tests ensure that serializable objects are properly white-listed or > excluded and are able to be serialized/deserialized. > > Excluded classes are not tested, though, so if you add an excluded class > and are wrong about whether it is sent over the wire you will break > Geode. Having serialization validation enabled in dunit tests protects > against this if you write tests that use the excluded class. > > We also have non-default values for cluster configuration in dunit runs. > > > On 3/13/18 10:03 AM, Kirk Lund 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 > > > >
