geode-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Kirk Lund <kl...@apache.org>
Subject Re: [PROPOSAL] use default value for validate-serializable-objects in dunit
Date Fri, 21 Dec 2018 22:42:23 GMT
<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 <bschuchardt@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
> >
>
>

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