bug#26803: [PATCH 16/36] gnu: Add java-commons-lang.

[Roel Janssen <roel@gnu.org>]

Ricardo Wurmus writes:

> Roel Janssen <roel@gnu.org> writes:
>
>> Ricardo Wurmus writes:
>>
>>> From: Hartmut Goebel <h.goebel@crazy-compilers.com>
>>>
>>> * gnu/packages/java.scm (java-commons-lang): New variable.
>>>
>>> Co-authored-by: Ricardo Wurmus <rekado@elephly.net>
>>> ---
> […]
>>> +    (arguments
>>> +     `(#:test-target "test"
>>> +       #:phases
>>> +       (modify-phases %standard-phases
>>> +         (add-after 'build 'build-javadoc ant-build-javadoc)
>>> +         (add-before 'check 'disable-failing-test
>>> +           (lambda _
>>> +             ;; Disable a failing test
>>> +             (substitute* "src/test/java/org/apache/commons/lang/\
>>> +time/FastDateFormatTest.java"
>>> +               (("public void testFormat\\(\\)")
>>> +                "public void disabled_testFormat()"))
>>> +             #t))
>>
>> Since you're renaming the function, I suppose removing the function
>> would also work, which would not create any "dead code".  But that
>> probably requires a separate patch file, which will break more easily
>> on an update.
>>
>> So long story short:  This is OK to me, even though I don't like
>> producing code that won't be run anyway.
>
> I agree in principle, but this is the Java way of disabling a test.
> Tests are considered tests only when the procedures start with the
> string “test”, so renaming the test causes it not to be executed.
> Removing requires much more effort because it requires an actual patch.
> Since it’s just a test (and not library code) I think it’s acceptable
> not to delete it.

I agree. So, LGTM!

Kind regards,
Roel Janssen