Re: [PATCH] Resolve testsuite interruption in SVN 1.8

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