Thank you both! As my understanding is that it will be more involved to deal with the per version config loading and that is the reason it was not done yet but a flag was added, I suggest we opt in for the flag and revise again this after the release. Let’s add it to the other topics we discussed around the in-jvm framework.
Two points to support my suggestion: - I just checked and there were no new in-jvm upgrade tests added since February, before that October and July last year in trunk. 9 months nothing added in the older branches. So it seems as a rare thing and just one flag to add to the test when writing it. - on trunk the old config (pre-15234) is read into the new types so by testing with it we actually exercise the whole path now - backward compatibility + load into the new types. Let me know if you have any concerns or questions. I will leave the discussion open until tomorrow and in case there are no concerns - I will assume lazy consensus and open a ticket to flip the flag. On Mon, 4 Apr 2022 at 13:41, David Capwell <dcapw...@apple.com> wrote: > 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> > 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 >> > >
