[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] tests/examples: Add running of workflow examples
From: |
Olivier Dion |
Subject: |
Re: [PATCH] tests/examples: Add running of workflow examples |
Date: |
Fri, 29 Apr 2022 17:05:50 -0400 |
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