[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [lmi] [PATCH] New input validation test
From: |
Greg Chicares |
Subject: |
Re: [lmi] [PATCH] New input validation test |
Date: |
Fri, 27 Mar 2015 00:55:12 +0000 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Icedove/31.3.0 |
On 2015-03-13 00:26, Vadim Zeitlin wrote:
[...]
> On Thu, 13 Nov 2014 03:28:13 +0000 Greg Chicares <address@hidden> wrote:
[...]
> GC> Here's how we'd like to change that test so that it encompasses
> GC> everything that is at present done manually. First, open an
> GC> '.ill' file that we'll provide to replace "CoiMultiplier.cns".
>
> The attached patches rely on the presence of CoiMultiplier.ill file in the
> GUI tests data directory.
IIRC (and I'd better), the idea here is to use a proprietary product
that permits a COI multiplier but requires it to be within a certain
range that varies by password.
> GC> Its parameters will all be defaults, except that we'll choose a
> GC> group executive carveout product.
Good, I did recall correctly.
> For testing, I used the default ("sample") product, but I changed the
> value of MinInputCoiMult in sample.database to 0.9.
Sure, that's the right way for you to do it locally.
> GC> Then we'll force values (a-j)
> GC> above into the
> GC> <object class="InputSequenceEntry" name="CurrentCoiMultiplier">
> GC> field, simulating "OK" after each one, and test the outcome
> GC> against the diagnostics that Input::RealizeCurrentCoiMultiplier()
> GC> prescribes.
>
> This is exactly what the current patches do. Please notice that the first
> patch in the series is a pure refactoring,
Committed 20150327T0027Z, revision 6152.
> the second one is a trivial new
> class addition
Committed 20150327T0032Z, revision 6153.
> and the only significant patch is the last one, which
Committed 20150327T0041Z, revision 6154.
> updates the input_validation test code and specification. The code is a bit
> long due to the presence of a couple of special cases, but I don't see any
> simple way to improve it, so I commented it to at least explain what is
> going on here instead.
>
> If the code complexity bothers you (it does bother me, FWIW), then I'd
> like to note that good part of it comes from the need to deal with (and
> check for) domain_error exception thrown for the negative COI multiplier
> when running with --mellon option. If we could avoid this test (for the
> value "-1"), the modifications to SkeletonTest would be unnecessary and the
> code of the test itself could be also simplified. Of course, this is not a
> good reason to drop/skip this test if it is valuable, but if it is not, it
> might be a good idea.
It feels right to me. I think the code being tested is right to distinguish
domain_error in this case, and I don't mind that the code to test it is
therefore a little more complicated.
Now we're about to add a new product with different restrictions on this
'CurrentCoiMultiplier' field. This unit test will help us guard against
regression errors in this new work.
And now I think I've applied all your 2015 patches except for the census
manager changes (which I must defer) and the wxxrc explicit-size removal
(which I will look into soon (especially because it might help me, with
my high DPI setting) as I need to add some new input fields anyway).