Re: [lmi] GUI test: 'paste_census' update

[Vadim Zeitlin]

On Mon, 14 Dec 2015 19:40:03 +0000 Greg Chicares <address@hidden> wrote:

GC> I've updated 'pasting_to_a_census.html', and would like to update its
GC> unit test. I started out by replacing the example in the unit test with
GC> the new user-manual example, but the kludges I had to add to make it
GC> work gave me pause--see below.

 Sorry if I'm missing something obvious, but which of the changes do you
object to in particular? I don't see any really bad here compared to the
existing code.

GC> Index: wx_test_paste_census.cpp
GC> ===================================================================
GC> --- wx_test_paste_census.cpp        (revision 6443)
GC> +++ wx_test_paste_census.cpp        (working copy)
GC> @@ -214,21 +214,20 @@
GC>      // internal column names actually used in the census data below.
GC>      std::set<std::string> column_titles;
GC>      column_titles.insert("Gender");
GC> -    column_titles.insert("Underwriting Class");
GC> -    column_titles.insert("Issue Age");
GC> -    column_titles.insert("Payment");
GC> -    column_titles.insert("Death Benefit Option");
GC> +    column_titles.insert("Date Of Birth");
GC> +    column_titles.insert("Employee Class");
GC> +    column_titles.insert("Specified Amount");
GC> 
GC>      char const* const census_data =
GC> -        
"Gender\tUnderwritingClass\tIssueAge\tPayment\tDeathBenefitOption\n"
GC> +        "Gender\tDateOfBirth\tEmployeeClass\tSpecifiedAmount\n"

 We could avoid having to make both of the changes above by extracting the
column titles from the first line of the data, but I decided against doing
it when writing this code because I thought it was more important to keep
it simple and I still believe it would be better to keep it like this.

GC> -        "Female\tPreferred\t30\tsevenpay,7;0\tb,7;a\n"
GC> -        "Male\tPreferred\t35\tsevenpay,7;0\tb,7;a\n"
GC> -        "Female\tStandard\t40\tsevenpay,7;0\tb,7;a\n"
GC> -        "Male\tStandard\t45\tsevenpay,7;0\tb,7;a\n"
GC> -        "Female\tPreferred\t50\tsevenpay,7;0\tb,7;a\n"
GC> -        "Male\tPreferred\t55\tsevenpay,7;0\tb,7;a\n"
GC> -        "Female\tStandard\t60\tsevenpay,7;0\tb,7;a\n"
GC> +        "Female\t19851231\tClerical\t100000, @65; 50000\n"
GC> +        "Male\t19801130\tClerical\t200000, @65; 50000\n"
GC> +        "Female\t19751029\tTechnical\t300000, @65; 50000\n"
GC> +        "Male\t19700928\tTechnical\t400000, @65; 50000\n"
GC> +        "Female\t19650827\tSupervisor\t500000, @65; 50000\n"
GC> +        "Male\t19600726\tAttorney\t600000, @65; 75000\n"
GC> +        "Female\t19550625\tPresident\t700000, @65; 100000\n"

 This change doesn't seem to raise any questions at all.

GC> @@ -301,8 +300,12 @@
GC>              LMI_ASSERT(gender_radiobox);
GC> 
GC>              wxUIActionSimulator ui;
GC> -            ui.Char(WXK_DOWN); // Select the last, "Unisex", radio button.
GC> +            // Select the last, "Unisex", radio button, by simulating
GC> +            // down-arrow twice: female --> male, then male --> unisex.
GC> +            ui.Char(WXK_DOWN);
GC>              wxYield();
GC> +            ui.Char(WXK_DOWN);
GC> +            wxYield();

 This one is ugly and it would be better if wxUIActionSimulator::Select()
could work with radio boxes too, but currently it doesn't because they're
completely different from list/combo boxes internally for which that method
was written.

GC> @@ -337,7 +340,8 @@
GC>      unsigned int const
GC>          gender_column = find_model_column_by_title(list_window, "Gender");
GC>      LMI_ASSERT_EQUAL(list_model->GetCount(), number_of_rows);
GC> -    for(std::size_t row = 0; row < number_of_rows; ++row)
GC> +    // Only the first two rows are affected. This seems fragile.
GC> +    for(std::size_t row = 0; row < 2; ++row)
GC>          {
GC>          wxVariant value;
GC>          list_model->GetValueByRow(value, row, gender_column);

 Again, this is definitely not particularly beautiful, but I don't think
it's worth extracting the number of rows having the same class as the first
row from the census data here. All I would change would be to make the
comment more explicit, saying that we check only the first two rows because
they are the only ones that use the class that we have just edited. But
other than that, what can we do?

 Sorry if I completely missed the point of your email (as I suspect I did),
but the only unambiguous improvement I can see here would be to use
Select("Unisex"), once it's made to work with radio boxes too, instead of
explicit Char(WXK_DOWN).

 Please let me know if you see any others,
VZ