I am cool with checking by default and disabling for tests that need it, it may also make more sense to add an allow list so tests can explicitly say which configs to ignore (though this sounds painful to implement).
> On Apr 4, 2022, at 9:11 AM, Jon Meredith <jmeredit...@gmail.com> wrote: > > I think checking the validation rules as part of testing is important, but > making that a per-test concern for handling it may be frustrating. > > What do you think about extending in-JVM with a method that provides the > previous config and version and expects the Instance implementation to > upgrade it, and then leave the validation on? That would provide some > documentation of how deprecated/removed options are expected to be migrated. > > It might also be worthwhile extending the upgrade tests so that we can also > specify new configuration in addition to migrated configurated as the > instance is upgraded. > > On Mon, Apr 4, 2022 at 8:43 AM Ekaterina Dimitrova <e.dimitr...@gmail.com > <mailto:e.dimitr...@gmail.com>> wrote: > Hi everyone, > In In-jvm tests there is a flag Constants.KEY_DTEST_API_CONFIG_CHECK to > opt-in/out from config checks done in YamlConfigurationLoader#check(). > The upgrade tests are currently set to ignore that check in order to work > around dealing with new properties in newer versions. What does this mean? My > understanding is that the lowest version from which we start an upgrade test > will load Config and use it for all versions. This means that our checks will > fail because in newer versions we set new config in InstanceConfig that > doesn't exist in the old version Config. If we opt in, the tests will start > failing because we need to remove parameters. > > I suggest we opt in by default to the checks so people consciously add their > config. What do I mean? Currently with the new types and names in trunk, we > exercise the backward compatibility and we set the old config names and > values that work with the previous versions and exercise the backward > compatibility but If I add a new name to set for config, this will just be > ignored silently and default Config is used. Test might even pass...This is > risky. > This was documented but I think the right course of action is to opt in for > checks and people ignore the checks in upgrade tests when they are sure they > add the right config and no typos, etc and they understand the implications. > The situation since that check was added has changed and if we keep on adding > more tests, I think this is important so we are sure we test properly. > > Please let me know if I am wrong in my understanding and what you think. > > Best regards, > Ekaterina
