[bug#59487] [PATCH v2 1/2] build-system/dune: Automatically deduce test-target in most cases.

[Csepp]

pukkamustard <pukkamustard@posteo.net> writes:

> Thanks for this! I think it is a valuable improvement.
>
> Csepp <raingloom@riseup.net> writes:
>
>> From: raingloom <raingloom@riseup.net>
>>
>> guix/build-system/dune.scm (dune-build): tests? defaults to #f.
>
> This should be: "test-target defaults to #f".
>
>> +    (let ((program (if jbuild? "jbuilder" "dune"))
>> +          (test-target (or test-target
>> +                           (cond
>> +                            ((file-exists? "tests") "tests")
>> +                            ((file-exists? "test") "test")
>> +                            (else ".")))))
>> +      (apply invoke program "runtest"
>> +             (append (if test-target (list test-target) '())
>> +                     (if package (list "-p" package)
>>                           dune-release-flags)
>>                       test-flags))))
>>    #t)
>
> I think what Julien ment (and I agree) is that you can completely drop
> the checks for the files/directories "tests" or "test" to exist.
>
> In your patch, if test-target is #f and "test" or "tests" do not exist,
> then the we will run:
>
> `dune runtest -p package .`
>
> but we could (and maybe should) run just:
>
> `dune runtest -p package`
>
> In fact, we should run this even if the "test" or "tests" directories
> exist (otherwise we might miss running some tests placed in other
> directories).
>
> So this would be enough:
>
>> +    (let ((program (if jbuild? "jbuilder" "dune")))
>> +      (apply invoke program "runtest"
>> +             (append (if test-target (list test-target) '())
>> +                     (if package (list "-p" package)
>>                           dune-release-flags)
>>                       test-flags)))
>
> Thinking of this, maybe we can drop the `test-target` argument
> completely. wdyt?
>
> -pukkamustard

Hmm, makes sense.  I'll test what complete removal does.