Re: [lmi] wx_test_input_sequences.cpp

[Greg Chicares]

On 2015-03-04 22:35, Vadim Zeitlin wrote:
> On Fri, 12 Dec 2014 15:17:49 +0000 Greg Chicares <address@hidden> wrote:
> 
> GC> // ERASE THIS BLOCK COMMENT WHEN IMPLEMENTATION COMPLETE. The block
> GC> // comment below changes the original specification, and does not
> GC> // yet describe the present code. Desired changes:
> GC> //  - Hard code the sequences; get rid of 'InputSequences.cns'.
> GC> //  - Paste each test sequence into a temporary input dialog.
> GC> //  - Validate with ellipsis button and then with OK.
> 
>  The attached patch implements the desired changes.

I committed the revised patch yesterday:
  http://lists.nongnu.org/archive/html/lmi/2015-03/msg00021.html

> There have been no
> particular problems with doing this, however it turned out there was a
> conflict between this test and the "default update" one and it needs to be
> solved somehow, please see the end of this message.

That's a common thread in several discussions, so I'll try to
tie everything together below.

> --- mostly informative part starts here ---
> 
> On Sun, 14 Dec 2014 15:12:12 +0000 Greg Chicares <address@hidden> wrote:
> 
> GC> BTW, we may test a variety of UI "skins" that use different sets of
> GC> input fields, and place them on various tabs. The "master" skin is
> GC> 'skin.xrc': it is intended to contain every input field (and probably
> GC> does, thought I haven't verified that recently). Other skins may lack
> GC> some fields.
> 
>  Just for the reference, the fields used (SpecifiedAmount, PaymentMode,
> Payment and Withdrawal) are present by all skins with the exception of
> skin_variable_annuity.xrc which doesn't have SpecifiedAmount and
> skin_single_premium.xrc which doesn't have neither PaymentMode nor
> Withdrawal.
> 
> GC> ...so whenever an individual sequence-paste test cannot be run because
> GC> of these reasons, I believe we should skip it silently.
> 
>  I've interpreted the word "individual" above as meaning that if one of the
> fields doesn't exist in the current skin, we should skip only testing the
> input sequence(s) which is/are supposed to be used with this field, but
> still continue testing the other ones -- and this is what the patch does.

Perfect. That's exactly what we want.

> Personally I think it's not the best idea because you have no idea at all
> if the tests were run or not, if the entire test were skipped there would
> at least be a mention of it in the output ("skipped (missing field in the
> skin)" or something like this), but currently there is no indication of it
> at all. So if, e.g. "Payment" field is ever renamed, more than half of the
> sequences wouldn't be tested any longer, but the test would still be
> reported as passing. I guess you can always say that you would notice this
> by seeing that the test execution time has suddenly drastically decreased,
> but I can't really convince myself that it's a good idea to rely on timings
> to check if the tests were run or not.

First, let's recapitulate some background information. Class Input contains
the whole universe of input fields. The several skins contain various subsets
of that universe.

By design, 'skin.xrc' contains the union of all fields present in any skin.
That union should be the universe of input fields minus a few that we choose
not to present in any skin--e.g., because they're obsolete, intermediate
dependent variables, or merely auxiliaries to support caching.

We'll frequently run the GUI test with 'skin.xrc', so it doesn't matter that
other skins lack some input-sequence fields: as long as 'skin.xrc' has them
all, they'll all be tested.

But you're asking what happens if we change the name of a field. Right now,
the test would treat that field as though it didn't exist: a test that had
always done something would silently begin to do nothing. I think it's a
good idea to guard against that by making sure the field name is still part
of the universe to which it originally belonged. Untested idea:

            if(!wxWindow::FindWindowByName(test_data_.field, dialog))
                {
                // If the field for this input sequence doesn't exist in the
!               // currently used skin at all, then make sure it's a valid
!               // field, and then skip this particular sequence silently --
!               // but continue testing the other ones.
!               input[field]; // Look it up in some global Input instance,
!               // in some way that will throw an exception if not found.
                return wxID_CANCEL;
                }

I believe indexing class Input by a name it doesn't contain calls
  complain_that_no_such_member_is_ascribed()
which throws. That's just one way of checking this invariant, and you may
well find another you like better. Or you might prefer to write a separate
routine to check each field name--there are many ways to do it, and I've
simply picked one that seems simple to present.

>  To summarize, I believe I've implemented what you requested, but please
> let me know if I misunderstood you or if you've been swayed by my arguments
> above and changed your mind and would like me to indicate skipping these
> tests in some way.

Sure, that works too:

                // If the field for this input sequence doesn't exist in the
!               // currently used skin at all, then say so, and skip this
!               // particular sequence silently -- but continue testing the
!               // other ones.
!               appropriate_output_stream << "Field " << field << " skipped 
because...";
                return wxID_CANCEL;

Not quite as in-your-face as the preceding idea, but good enough:
I'd be sure to notice if it fails to find a field in 'skin.xrc'.

Or I suppose you could read back the value of the modified field, e.g.
input["Payment"], to make sure it actually was modified.

> --- the important part part starts here ---
> 
> GC> "SpecifiedAmount"  sevenpay 7; 250000 retirement; 100000 #10; 75000 @95; 
> 50000
> 
>  This test case clashes with the "default update" test I implemented
> earlier today (this was a happy coincidence, it took me half an hour to
> understand what was going on even when it should have been obvious to me
> as the other test was still fresh in my memory and I would have probably
> spent twice as long on it if I were doing it in a couple of weeks after
> forgetting everything about it...).
> 
>  The problem is that "default update" test changes the default value of
> DateOfBirth which also changes the default IssueAge, from 45 to 58.

I thought I was fifty-nine.

> And as
> the retirement age remains at 65, this means that "retirement" keyword
> corresponds to a duration of 7, and so the second interval overlaps with
> the first one, resulting in the sequence validation error (BTW, the fact
> that this other 7 was coming from the difference between the retirement and
> issue ages was really not obvious to me, perhaps this would have been much
> more obvious for the real lmi users, but I think that improving the error
> message might still be helpful).

Any diagnostic can always be improved; but this one seems obvious enough to
our end users, and I'm not willing to revisit that code now.

>  I see several ways of solving this problem, but the one I strongly prefer
> would be to avoid modifying the defaults in the "default update" test in
> the first place. You did already unequivocally reply that that test
> shouldn't be preserving the defaults file and I even managed to restrain
> myself from arguing with it when I posted my patch to it earlier today, but
> I can't avoid bringing it up again now.

Okay, you've convinced us. Please go ahead and modify that test so that it
preserves the default input file essentially (see below) unchanged. This
addresses the following concerns you've raised in two other threads:

http://lists.nongnu.org/archive/html/lmi/2015-03/msg00019.html
Re: [lmi] wx_test_default_update.cpp
>  Please notice that as a consequence of this decision, the test is "run
> once": i.e. while it runs successfully when it's launched the first time,
> it will fail, because there will be no changes to save in the defaults

http://lists.nongnu.org/archive/html/lmi/2015-03/msg00023.html
Re: [lmi] [PATCH] wx_test_calculation_summary.cpp
> AFAICS it does follow it faithfully, but there are 2 remaining
> questions/potential problems: first one is the previously mentioned issue
> with getting the default summary columns

Right now, the narrative description reads:

/// Make sure the default input file can be opened, modified, and saved.
...
/// Change its "DateOfBirth" option. This particular option is used because it
/// is available for any life insurance product as the date of birth is a field
/// of such central importance.
///
/// Save the changed file; make sure the appropriate message appears
/// on the status bar. Make sure the saved file exists in its
/// configured directory.

Because this file is "special" and has its own commands, I do hesitate to
lose the value added by the last paragraph. I.e., if we ever make a mistaken
code change that breaks the ability to save it with modifications, then it
would be really nice to have the GUI test tell us. Suppose we do this:

- Keep the code that changes "DateOfBirth", because that's an excellent
  example of the sort of field a user might want to change here, and test
  that the change occurred...

- But then change "DateOfBirth" and "UseDOB" back to their original values,
  and make an arbitrary change to "Comments" (e.g., write a timestamp into
  "Comments" with microsecond precision, which must be unique because the
  test takes more than one microsecond). As long as your arbitrary change
  doesn't contain the substring "idiosyncrasy", it should have no effect on
  any other test: it leaves the file *essentially* unchanged.

- Then save the file, testing for success as stated in the last paragraph of
  the above specifications.

How does that sound?