unofficial mirror of gwl-devel@gnu.org
 help / color / mirror / Atom feed
From: Ricardo Wurmus <rekado@elephly.net>
To: Olivier Dion <olivier.dion@polymtl.ca>
Cc: gwl-devel@gnu.org
Subject: Re: [PATCH] tests/examples: Add running of workflow examples
Date: Fri, 29 Apr 2022 22:32:08 +0200	[thread overview]
Message-ID: <87sfpvzkmj.fsf@elephly.net> (raw)
In-Reply-To: <20220429175500.17176-1-olivier.dion@polymtl.ca>


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.

> +  (define scenarios
> +       (list "extended-example-workflow.w"))

Should these better be discovered automatically via SCANDIR?

> +  (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))

?

> +                  (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.

> +                      (format (error-output-port)
> +                              "Example directory: ~a\n" tmp-dir))

Nitpick: ~% instead of \n.

> +                  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.

-- 
Ricardo


  reply	other threads:[~2022-04-29 20:43 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-04-29 17:55 [PATCH] tests/examples: Add running of workflow examples Olivier Dion
2022-04-29 20:32 ` Ricardo Wurmus [this message]
2022-04-29 21:05   ` Olivier Dion via
2022-05-07  6:46     ` Ricardo Wurmus
2022-05-07 16:47 ` [PATCH v2] " Olivier Dion
2022-06-03  9:06   ` Ricardo Wurmus
2022-06-03 13:11     ` Olivier Dion via
2022-06-03 15:46       ` Ricardo Wurmus

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: https://www.guixwl.org/

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87sfpvzkmj.fsf@elephly.net \
    --to=rekado@elephly.net \
    --cc=gwl-devel@gnu.org \
    --cc=olivier.dion@polymtl.ca \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).