Re: [lmi] wx_test_input_sequences.cpp

[Vadim Zeitlin]

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. 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.


--- 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.
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.

 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.

GC> Here's a complete list of input sequences from the user manual, with
GC> a numbers-only sequence added, showing the field (in .xrc files, the
GC> "name=" identifier) to use for each.

 Thanks, this was very useful.


--- 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. 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).

 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. As you can see from the conflict
between the tests described above, changing the defaults can result in
quite non obvious effects on the other tests. I'm quite sure this is not
going to be the last place where this problem is going to manifest itself.
For the stability of the tests and our own sanity it would be greatly
preferable to avoid modifying the characteristics of all the illustrations
created by the tests midway through the test suite.

 So could we please change the "default update" test to avoid permanently
modifying the file? If yes, I'll make the necessary changes to it and
submit them as another patch -- and then you could apply patch attached to
this message without any changes. If not, this patch would need to be
modified to work with the alternative DateOfBirth value, e.g. by changing
the first interval duration or changing the date of birth explicitly for
this illustration or in some other way.

 Thanks in advance,
VZ

0001-Implement-the-updated-input-sequences-test-specifica.patch
Description: Text document