Re: [PATCH] Resolve testsuite interruption in SVN 1.8

On Sun, Aug 7, 2016 at 10:14 PM, Stefan <luke1410_at_posteo.de> wrote:
> On 8/7/2016 21:16, Ivan Zhakov wrote:
>> On 7 August 2016 at 21:27, Stefan Sperling <stsp_at_elego.de> wrote:
>>> On Sun, Aug 07, 2016 at 08:23:58PM +0200, Stefan Sperling wrote:
>>>> On Sun, Aug 07, 2016 at 02:08:35PM +0200, Stefan wrote:
>>>>> Hi,
>>>>>
>>>>> the attached patch is the outcome of talking with jcorval on IRC about a
>>>>> test suite issue on Windows (release builds) in SVN 1.8. It resolves the
>>>>> fact that the tests will be interrupted on Windows by a Windows popup
>>>>> upon an abort()-call.
>>>>>
>>>>> Atm this is triggered for the 1.8 test suite for the move-test (no 8)
>>>>> which is marked as XFail and triggers an SVN_ERR_ASSERT() and therefore
>>>>> breaks fully automated tests.
>>>> I'm not a windows person but this looks reasonable to me. +1
>>> Forgot to mention: If this is a 1.8-only fix, I'd suggest you create
>>> a branch of 1.8.x and commit your patch there, and then nominate your
>>> branch in the 1.8.x/STATUS file (you can add my +1 to the nomination).
>> I'm not sure that we should add handling of new environment variable
>> in patch release.
>>
>>
> I can understand the concern. Please bare in mind however that the
> default behavior (aka: if the environment variable is not set) would
> stay unchanged and the effect of the environment variable is quite
> limited (it will only disable the Watson crash dumps in release builds -
> obviously only applies on Windows builds). It's by default only
> activated for the test suites and therefore the risks involved is IMO
> close to non existent.
>
> Weighting the pros (fixing a test interruption of the test suite for
> everybody testing/building the SVN package) with the cons (the risk of
> s/o unintentionally already having set an environment variable with the
> same name and on the other side introducing a new test-suite related
> feature in a patch release which would be not available in versions <=
> 1.8.16), I do have a tendency to still propose that patch. (Note: I
> would suggest otherwise, if it wasn't related to a fix for the
> testsuite, since then it could hardly be seen as a bugfix).
>
> I thought about alternatives, but rejected them due to different reasons:
>
> Alternative 1:
> Do not use the environment variable and rather make it implicit behavior
> to disable the Watson crash reports.
> Rejected, since that would be a behavior change of SVN. Downstream
> releases of SVN might rely on current crashdump integration and would be
> broken.
>
> Alternative 2 (untested):
> For the test environment set the corresponding Windows registry entry
> [1] prior to running the SVN tests so to disable sending the
> crashreports and/or bring up the popup to attach a debugger.
> Rejected, since that would be an additional barrier to run the testsuite
> especially for people building SVN themselves. IMO there should be as
> few hurdles as possible to build and test SVN, so to not risk stopping
> people from giving SVN a try.
>
> Alternative 3:
> Use a config setting instead of an environment variable.
> Rejected, since there we could not disable the crashreports before the
> config file was processed.
>
> Alternative 4:
> Use a command line parameter to disable crashreports.
> Rejected, since it would only work for command line tests but not for
> testing the C API.
>
> Maybe your call, Ivan, would be to use alternative 2 for SVN <= 1.9 and
> introduce the environment variable related fix in trunk only?
>
> [1]
> https://blogs.msdn.microsoft.com/alejacma/2011/02/18/how-to-disable-the-pop-up-that-windows-shows-when-an-app-crashes/
>
> Regards,
> Stefan

Thanks for taking this on, Stefan. This has been bothering me every
time I needed to do release testing/signing for 1.8.x (but I didn't
know how to fix it, and didn't have the energy to push it further
myself).

I'm +1 for your current fix, certainly on trunk, but also for backport
to 1.8 and 1.9 (given your arguments above).

That said, I can also live with other solutions. In fact, currently
there is only this one known test crash in 1.8 (move-test #8), and no
known test crashes in 1.9. So if it's too contentious to backport
this, I can live with knowing about that registry entry (where I can
"fix" it myself -- thanks for pointing that out!) ... there are not
that many people who will still test 1.8 on Windows. New interested
contributors will most likely start with trunk (hopefully with your
fix then) or 1.9 (no known test crashes).

-- 
Johan