Re: [PATCH] tests/examples: Add running of workflow examples

[Olivier Dion via <gwl-devel@gnu.org>]

On Fri, 29 Apr 2022, Ricardo Wurmus <rekado@elephly.net> wrote:
> Hi Olivier,
>
> thanks for the patch!
>
>> End to end testing of pre-defined scenarios are a good way to check for
>> regression.
>>
>> Here we introduce testing of some examples available in the documentation.  That
>> way, we're sure that new users should be able to run them without problems.
>>
>> Each scenario is a different test and is run in a different temporary
>> directory which get destroyed if the scenario succeeded.
>
> It’s a good idea to run the examples.  However, this requires a fully
> functional Guix installation (which we don’t have when building with
> Guix, for example), so we should not include them in “make check” but
> add a separate target for these tests.  An alternative may be to do what
> Guix does for system tests, but I’d be okay with just having a separate
> target that is run manually before releases or in CI.
>
>> +(define-syntax-rule (with-directory-excursion dir body body* ...)
>> +  (let ((old-dir (getcwd)))
>> +    (dynamic-wind
>> +      (lambda () (chdir dir))
>> +      (lambda () body body* ...)
>> +      (lambda () (chdir old-dir)))))
>> +
>> +(with-directory-excursion
>> +    (string-append top-srcdir "/doc/examples")
>> +
>> +  (define (process-success? status)
>> +    (= 0 (or (status:exit-val status)
>> +             (status:term-sig status))))
>
> Use ZERO? here.

I learn everyday with you.  I did not know about it ^^.

>> +  (define scenarios
>> +       (list "extended-example-workflow.w"))
>
> Should these better be discovered automatically via SCANDIR?

Well some example are not full workflow.  Only processes.  So I think that
yes, if we set a common prefix for full workflow:

(scandir "." (cut string-prefix "exemple-workflow" <>))

However, it's nice -- even more true when debugging -- to be able to
cherry-pick the tests.  This is easier with the list above by commenting
tests that are passing.

>> +  (for-each (lambda (example)
>> +              (test-assert example
>> +                (let* ((tmp-dir (mkdtemp
>> +                                 (format #f "gwl-example-~a.XXXXXX" example)))
>> +                       (abs-example (canonicalize-path example))
>> +                       (success?
>> +                        (with-directory-excursion tmp-dir
>> +                          (process-success?
>> +                           (system
>> +                            (format #f "guix workflow run -fc ~a -l all"
>> +                                    abs-example))))))
>
> Please don’t use SYSTEM.  How about
>
>     (system* "guix" "workflow" "run" "--force" "--container"
>              "--log-events=all" (canonicalize-path example))

Yes I like it better too thanks.

>> +                  (if success?
>> +                      (system* "rm" "-rf" tmp-dir)
>
> Why shell out to RM when we have DELETE-FILE and its recursive friend in
> Guix?  I’d also rather move clean-up work to a DYNAMIC-UNWIND handler.

I hesitated to include Guix's module in test.  If you're okay with it, I
will use the helpers available in Guix.  I concur that the cleanup
should be in dynamic-unwind.

>> +                      (format (error-output-port)
>> +                              "Example directory: ~a\n" tmp-dir))
>
> Nitpick: ~% instead of \n.

Is there a reason why?  I don't mind I just never really understood why
scheme has this special format rule for newline.

>> +                  success?)))
>> +            scenarios))
>
> This FOR-EACH loop combines test definition with test running, which
> seems wrong to me.  Maybe SRFI-64 is not the best fit for tests that
> only care about whether a shell command was run successfully.  Perhaps
> we should do as Guix does and just have a shell script to run these
> tests.

What do you find wrong about it?  We could re-write it as:

(define (run-example path)
  ...)

(test-assert (run-example "extended-workflow.w"))
(test-assert (run-example "..."))

if you like it better.  However, we lose the power of SCANDIR mentioned
above.



Let me know, I will send a v2 with your above recommendations and answers
to my questions.

-- 
Olivier Dion
oldiob.dev