Re: [lmi] GUI unit test failures after the recent changes

[Vadim Zeitlin]

On Fri, 13 May 2022 22:46:31 +0000 Greg Chicares <greg.chicares@gmail.com> 
wrote:

GC> On 5/13/22 16:02, Vadim Zeitlin wrote:
GC> > 
GC> >  Just in case it has somehow gone unnoticed, the recent changes (I'm not
GC> > sure which one exactly as I didn't have time to debug them yet, but it 
must
GC> > be one of the 4 of the latest commits as of time of this writing, i.e.
GC> > b90777476..fc01b15e6) have broken the GUI unit tests that now fail with:
GC> > 
GC> >   calculation_summary: started
GC> >   Assertion 'materially_equal(bourn_cast<double>(irate), rate * radix)' 
failed.
GC> >   [ul_utilities.cpp : 152]
GC> > 
GC> > (see e.g. https://github.com/let-me-illustrate/lmi/runs/6420838991 for the
GC> > 32-bit build or https://github.com/let-me-illustrate/lmi/runs/6420839080
GC> > for the 64-bit one).
GC> 
GC> I hadn't noticed--it works here:

 Just to confirm: it will also work in the CI builds once they use the
updated sample archive files (please see my other email that you should
have hopefully received now, after I pruned all the "suspicious" links from
it).

GC> >  Independently of the actual problem itself, there is also a problem with
GC> > assert reporting, as they currently show a message box which means they
GC> > remain stuck forever (or until 6 hours of the very generous time quota of
GC> > GitHub Actions expire). This is something I will try to fix a.s.a.p. as
GC> > these tests are not supposed to require any human interaction
GC> 
GC> Good.

 I've fixed this in https://github.com/let-me-illustrate/lmi/pull/208 which
uses xanadu/remove-illview-report-exception branch and I think this branch
should be merged in any case because the code removed by the commits in it
is completely unnecessary since 10+ years. And, also, even though I didn't
mention it in the commit message, it's buggy as well because if you try to
create an illustration using the bad sample product files right now, you
still get a "zombie view" because has_view_been_created remains true if
Run() throws an exception. So simply removing the code entirely performs
the task it's supposed to do better than having it in the first place.

 However this fixes just this particular problem and not the more general
problem which can still happen if report_exception() is called from other
places. I think the real solution would be to allow using a custom
safe_message_alert_function for the GUI test, but this would require some
changes to the core lmi code because currently you can't change this
function any more once it is set.

 Alternatively, we could return to the idea of using wxSafeShowMessage() in
safe_message_alert() in alert_wx.cpp: this function's behaviour is
customizable via wxAppTraits class and so we could

(a) Use the same ::MessageBoxA() call that we is used in lmi code right now
    even when using it under MSW -- which, of course, doesn't gain us
    anything under this platform, but would be nicer when using wxGTK.

(b) Override it to not show the message box in the GUI tests.

(c) Also use wxSafeShowMessage() instead of wxMessageBox() to avoid crashes
    under wxGTK, see https://github.com/let-me-illustrate/lmi/pull/167
    which has an uglier and less reliable workaround for it.

 
 To summarize:

1. Could you please merge PR 208, as it is the right thing to do anyhow.
2. Could you please let me know if you prefer the smallest possible change
   allowing to set the safe_message_alert_function from the GUI test or a
   more extensive but more general and fixing other problems with wxGTK
   change switching to wxSafeShowMessage()?

 Thanks in advance!
VZ

pgpPYFjKMfGVz.pgp
Description: PGP signature