Re: [lmi] census benchmark test (was: Automated GUI testing, revisited)

[Vadim Zeitlin]

On Fri, 21 Nov 2014 23:49:50 +0000 Greg Chicares <address@hidden> wrote:

GC> > GC> Here's more information, from another run:
GC> > GC> 
GC> > GC> 11:49:25: Run case for census_1: 416ms elapsed (expected -4.15%)
GC> > GC> 11:49:25:     benchmark_census: ERROR (Assertion 
'std::fabs(diff_in_percents) < 10' failed
GC> > GC> (expected 11000ms, but actually took 416ms, i.e. -96.22%).
GC> > GC> [file /lmi/src/lmi/wx_test_benchmark_census.cpp, line 101] )
GC> > GC> 
GC> > GC> Doesn't this suggest that the "ERROR" comparison is incorrect?
GC> 
GC> And here's another run, which reflects the menu-accelerator change:
GC>   http://svn.savannah.nongnu.org/viewvc?view=rev&root=lmi&revision=6036

 Sorry, but this change was incomplete: it updated the "pdf_census" test,
but not the "benchmark_census" one which we're discussing here.

GC> 22:29:03: Run case for census_1: 443ms elapsed (expected +2.07%)
GC> 22:29:03: Print to disk for census_1: 443ms elapsed (expected -95.97%)
GC> 
GC> This seems equivalent to the earlier results quoted above, in that
GC> the "elapsed" times are implausibly equal.

 Yes, this is just because the second operation ("print to disk") didn't
run at all and the test took the times of the previous operation. Please
apply this patch (or let me know if I should commit it myself) to fix this
and have more precise error messages in the future:
---------------------------------- >8 --------------------------------------
commit dc81b207b969c297aadd053e79a4236ba08f6e74
Author: Vadim Zeitlin <address@hidden>
Date:   Sun Nov 23 19:40:30 2014 +0100

    Clear status bar before running each command in census_benchmark unit test.
    
    This ensures that we recognize when running the command failed completely,
    instead of just reusing the timings of a previous test as we did before.

diff --git a/wx_test_benchmark_census.cpp b/wx_test_benchmark_census.cpp
index 7e2a0c0..ddd2b96 100644
--- a/wx_test_benchmark_census.cpp
+++ b/wx_test_benchmark_census.cpp
@@ -68,6 +68,9 @@ class census_benchmark
         ,int mod
         )
         {
+        // Clear any status text left over from the previous run.
+        status_.SetStatusText(wxString());
+
         wxUIActionSimulator z;
         z.Char(key, mod);
         wxYield();
@@ -127,7 +130,7 @@ class census_benchmark
         }
 
   private:
-    wxStatusBar const& status_;
+    wxStatusBar& status_;
     wxString const name_;
 };
 
diff --git a/wx_test_statusbar.hpp b/wx_test_statusbar.hpp
index 870114f..86789ba 100644
--- a/wx_test_statusbar.hpp
+++ b/wx_test_statusbar.hpp
@@ -35,7 +35,7 @@
 /// Return the status bar of the main window throwing an exception if anything
 /// goes wrong.
 
-inline wxStatusBar const& get_main_window_statusbar()
+inline wxStatusBar& get_main_window_statusbar()
 {
     wxWindow* const mainWin = wxTheApp->GetTopWindow();
     LMI_ASSERT(mainWin);
---------------------------------- >8 --------------------------------------

GC> >  Sorry if I'm missing something, but I don't see anything wrong with it.
GC> 
GC> I don't see how it's plausible for "Run case" and "Print to disk" to
GC> take exactly the same amount of time, over and over.

 Sorry, I was unclear. I meant that other than not using the correct
accelerator, i.e. not running the test at all, I didn't see anything wrong
and that I thought that fixing the accelerator was enough to fix the
problem. And I still do think this and here is the trivial patch which does
work for me:

---------------------------------- >8 --------------------------------------
commit 098b0b9a3fb97804e1314a5e90965c209c48b6d2
Author: Vadim Zeitlin <address@hidden>
Date:   Sun Nov 23 19:49:14 2014 +0100

    Update accelerator for "Print to PDF" command in census_benchmark unit test.
    
    Use the correct accelerator and also update the label used in the test 
success
    or failure message.
    
    Notice that the config file key is still called "time_disk" and not 
"time_pdf"
    and might need to be changed for consistency.

diff --git a/wx_test_benchmark_census.cpp b/wx_test_benchmark_census.cpp
index ddd2b96..c165838 100644
--- a/wx_test_benchmark_census.cpp
+++ b/wx_test_benchmark_census.cpp
@@ -167,9 +167,9 @@ class census_benchmark
         }
 
         b.time_operation
-            ("Print to disk"
+            ("Print to PDF"
             ,time_disk
-            ,'k'
+            ,'i'
             ,wxMOD_CONTROL | wxMOD_SHIFT
             );
 
---------------------------------- >8 --------------------------------------

GC> Yet I have made that change:
GC>   
http://svn.savannah.nongnu.org/viewvc/lmi/trunk/wx_test_pdf_create.cpp?root=lmi&r1=6036&r2=6035&pathrev=6036&diff_format=u
GC>      // Print the census to disk.
GC> -    ui.Char('k', wxMOD_CONTROL | wxMOD_SHIFT);  // "Census|Print case to 
disk"
GC> +    ui.Char('i', wxMOD_CONTROL | wxMOD_SHIFT);  // "Census|Print case to 
disk"
GC>      wxYield();

 Just to repeat what I already said above: this change is not enough, it's
correct, but it didn't fix the test we're speaking about here.

GC> I should mention that I have a local patch that is intended only to
GC> prevent this from aborting the test; but, AFAICT, it cannot account
GC> for this perceived anomaly:
GC> 
GC> Index: wx_test_benchmark_census.cpp
GC> ===================================================================
GC> --- wx_test_benchmark_census.cpp    (revision 6034)
GC> +++ wx_test_benchmark_census.cpp    (working copy)
GC> @@ -88,7 +88,7 @@
GC>                      / static_cast<double>(time_expected);
GC> 
GC>              delta.Printf("%+.2f%%", diff_in_percents);
GC> -
GC> +#if 0
GC>              LMI_ASSERT_WITH_MSG
GC>                  (std::fabs(diff_in_percents) < 10
GC>                  ,wxString::Format
GC> @@ -99,6 +99,7 @@
GC>                      ,delta
GC>                      )
GC>                  );
GC> +#endif // 0
GC>              }
GC>          else
GC>              {

 No, this patch shouldn't affect this, but OTOH it shouldn't be necessary
to disable this check neither...

 Anyhow, I hope this finally clears this particular issue, please let me
know if you have any questions and/or whether I should commit the patches
above or you prefer to do it yourself.

 Thanks,
VZ