Re: [lmi] Preserving configurable settings after GUI test

[Vadim Zeitlin]

On Tue, 10 Jul 2018 22:54:40 +0000 Greg Chicares <address@hidden> wrote:

GC> On 2018-07-03 00:01, Vadim Zeitlin wrote:
GC> > 
GC> >  There is a long standing TODO item about preserving the contents of
GC> > configurable_settings.xml file after running the GUI unit test, which
GC> > currently modifies it. The problem is that currently there doesn't seem to
GC> > be any simple and reliable way to do it using configurable_settings class
GC> > API: it doesn't allow neither copying it, which could be used to preserve 
a
GC> > backup copy, nor retrieving the underlying XML file path, which could be
GC> > used to make a backup copy of the file and restore it.
GC> > 
GC> >  I think that the possibility to do either one or the other needs to be
GC> > added, because the only other alternative I see is to use 
MemberSymbolTable
GC> > class API to make a member-by-member copy, which seems quite ugly. But I
GC> > have a lot of trouble guessing which of the approaches would you prefer.
GC> 
GC> This isn't an easy question.

 Yes, which is why, for once, I actually hoped you'd have some very firm
opinion about it, even if I'd find it very surprising. But my expectations
were thwarted again :-/

GC> > From my point of view, just exposing the XML file path would be the
GC> > simplest way to solve the problem, but it's also the least flexible one,
GC> > i.e. if configurable_settings ever changes to use some other source of
GC> > configurable information than a file (and I think this is perfectly
GC> > possible), the code in the unit test would need to be changed, while just
GC> > allowing to really copy the configurable_settings object contents would
GC> > still continue to work.
GC> 
GC> Initially I preferred this and wrote the implementation below. But now
GC> I'm not really sure. Is another storage mechanism really feasible?

 Yes, definitely.

GC> We can't use wxConfigBase for the command-line lmi binaries, not without
GC> introducing a dependency that we've studiously avoided all these years.

 Agreed, but we could implement some similar mechanism in lmi itself.

GC> We already use xml extensively, and it seems natural to use it here too.

 As much as I hate XML, I think it's not the weakest link here as it will
probably indeed be still there in 50 years (now I feel bad just from
thinking about how sad this really is). But XML doesn't necessarily need to
be stored in files.

GC> And our stated goal is that lmi should remain usable forty years after
GC> I retire, i.e., for another fifty or sixty years; I'm confident that
GC> flat files will still work then.

 I'm not. Right now some platforms (Android, maybe macOS, although I'm not
quite sure about it, and possible Windows RT) strongly encourage you to not
use them, but use some platform-specific configuration storage mechanisms
more suitable for sandboxed applications. I can only see this trend
continuing because of various non-technical advantages it is seen as
bringing. But, to be honest, storing all application configuration in, say,
a single SQLite database instead of a whole bunch of disk files, has
technical advantages too (see https://www.sqlite.org/appfileformat.html for
some arguments).


GC> >  So which one of the following would you prefer:
GC> > 
GC> > 1. Make private configuration_filepath() function public, i.e. a (static)
GC> >    member of configurable_settings.
GC> > 2. Make configurable_settings copy ctor and assignment operator public and
GC> >    implement them.
GC> 
GC> All data members are naturally copyable (no weird stuff like pointers), so
GC> maybe this would be easy. OTOH, the class derives from MemberSymbolTable,
GC> so it's not entirely trivial, but we can just clone
GC>   Input::Input(Input const& z)
GC>   Input& Input::operator=(Input const& z)
GC> so it's not really hard either. But OTTH, this is a Meyers singleton, and
GC> those special member functions are deliberately unimplemented to prevent
GC> anyone from using them by accident; that's a pretty strong reason to keep
GC> them private.
GC> 
GC> Perhaps a have-our-cake-and-eat-it-too solution is to implement them
GC> privately and extend friendship to its GUI test. Let's call that (2a) for
GC> ease of reference.

 FWIW if we do it like this, I'd rather do:

2b. Implement copying of this object using explicit methods, e.g. copy().
    This would prevent accidental cloning, but would still allow to make
    (and later restore) a copy of the object if really necessary.

GC> > 3. Use MemberSymbolTable methods directly.
GC> 
GC> That does seem yicky. We'd have to do all the work of (2a) anyway (i.e.,
GC> cloning class Input's special members would be the easiest way), so this
GC> boils down to implementing those special members under deviant names like
GC>   Foo::Copy(Foo const&)
GC>   Foo& Foo::Assign(Foo const&)
GC> which would be unnatural.

 I agree, let's discard this one.

GC> > or do you see some other approach?
GC> 
GC> 4. Right now, we have
GC>     void save() const; // public
GC>     void load();       // private
GC> and we could add overloads that take a nondefault filename. That seems
GC> easy enough, especially because the implementations are one-liners.

 I like the simplicity of this, although it's not quite as simple as
implied above because we also need to add the (optional) filename argument
to instance() and the ctor. The main drawback I see is that we'd actually
test the code in slightly different environment from the actual one and
this is, in general, not a great idea, even if I don't really see how could
it be a problem in this particular case.

GC> I'm not sure which of {1, 2a, 4} I like best. What do you think?

 My order of preferences would be [2b, 1, 4], but the differences between
all of them are small and I could live with any of them. (2a) would be
acceptable too but IMO it's strictly inferior to (2b).

GC> Here's my implementation of 1:

 The patch is a bit difficult to read because of moving around
default_calculation_summary_columns(), but I think it's perfectly fine. And
this would be definitely sufficient for the testing code to do its job. So
if you don't want to spend any more time on this, please do commit it.

 Thanks in advance,
VZ