Hi,

Squashed patch will come later on.

Maxim Cournoyer <maxim.cournoyer@gmail.com> writes:

> Hi,
>
> muradm <mail@muradm.net> writes:
>
> [...]
>
>> --- /dev/null
>> +++ b/gnu/tests/security.scm
>
> I'd keep the tests with the introductory commit (squashed in 
> preceding
> one).
>

Done.

[...]

>> +(define (run-fail2ban-basic-test)
>> +
>> +  (define os
>> +    (marionette-operating-system
>> +     (simple-operating-system
>> +      (service fail2ban-service-type))
>> +     #:imported-modules '((gnu services herd)
>> +                          (guix combinators))))
>                              ^ (guix combinators) seems unused
>

Done including other places.

>> +  (define vm
>> +    (virtual-machine
>> +     (operating-system os)
>> +     (port-forwardings '())))
>
> (define vm (virtual-machine (operating-system os))) should be
> sufficient.
>

For me it does not work without specfying port-forwardings.
I get wierd error like following:

gnu/tests/security.scm:47:5: error: os: invalid field specifier

I suppose it is something todo with virtual-machine.

So I'm leaving port-forwardings as is.

[...]

>> +          (define (wait-for-unix-socket-m socket)
>> +            (wait-for-unix-socket socket marionette))
>
> Overkill as used once in scope.
>

Done including other places.

>> +
>> +          (test-runner-current (system-test-runner #$output))
>> +          (test-begin "fail2ban-basic-test")
>> +
>> +          (test-assert "fail2ban running"
>> +            (marionette-eval
>> +             '(begin
>> +                (use-modules (gnu services herd))
>> +                (start-service 'fail2ban))
>> +             marionette))
>
> I like to test that services can be restarted too, as in my 
> experience
> there can be races and other situations that may cause them to 
> fail
> restarting.
>

Done.

[...]

>> +          (test-equal "fail2ban sshd jail running"
>> +            '("Status for the jail: sshd"
>> +              "|- Filter"
>> +              "|  |- Currently failed:\t0"
>> +              "|  |- Total failed:\t0"
>> +              "|  `- File list:\t/var/log/secure"
>> +              "`- Actions"
>> +              "   |- Currently banned:\t0"
>> +              "   |- Total banned:\t0"
>> +              "   `- Banned IP list:\t"
>> +              "")
>> +            (marionette-eval
>> +             '(begin
>> +                (use-modules (ice-9 rdelim) (ice-9 popen) 
>> (rnrs io ports))
>> +                (let ((call-command
>> +                       (lambda (cmd)
>> +                         (let* ((err-cons (pipe))
>> +                                (port (with-error-to-port (cdr 
>> err-cons)
>> +                                        (lambda () 
>> (open-input-pipe cmd))))
>> +                                (_ (setvbuf (car err-cons) 
>> 'block
>> +                                            (* 1024 1024 16)))
>> +                                (result (read-delimited "" 
>> port)))
>> +                           (close-port (cdr err-cons))
>> +                           (values result (read-delimited "" 
>> (car err-cons)))))))
>> +                  (string-split
>> +                   (call-command
>> +                    (string-join (list #$%fail2ban-server-cmd 
>> "status" "sshd") " "))
>> +                   #\newline)))
>> +             marionette))
>
> Perhaps this could be turned into an Shepherd action, and the 
> Guile
> procedure could do the above to return the text output; to 
> simplify the
> test and reduce boilerplate, while providing value to the user.
>

[...]

>> +  (gexp->derivation "fail2ban-extending-test" test))
>> +
>> +(define %test-fail2ban-extending
>
> Perhaps %test-fail2ban-extension ?

Done, s/extending/extension/.

> Otherwise, that last test seems to
> test exactly the same things as the preceding one, so there 
> should be a
> procedure to generate the test, taking the OS as an argument to 
> avoid
> code duplication.
>

Done, refactored with define-syntax-rule.

> Thanks for working on this!
>
> Maxim