Add a few integration tests using shellspec.

[Sjlver]

These test basic cases for the add, del, list, and edit commands. They are in no way comprehensive, but this could be a good start.

[Sjlver]

@biox Let me know if you're interested in something like this. If yes, we can potentially share the load of creating a more comprehensive test suite.

[Sjlver]

To run them, install https://github.com/shellspec/shellspec:

[biox]

Thank you for your work on this - I think it lines up well with the spirit of the project. I'm a little torn. On one hand, automated spec tests would be useful - on the other hand, it does mean introducing several dependencies - from shellspec, to whatever CI system we choose to run the shellspec. pa is so small that regressions are normally caught through standard usage, so I'm leaning towards "this is cool and useful, and fits the spirit of pa, but might not be something we want to adopt & maintain long-term"

do you have thoughts about this @arcxio?

[Sjlver]

Thanks for the thoughts!

I think that you don't necessarily need to run this as part of CI. Doing so would be nice (and also quite easy and cheap, using GitHub Actions), but I don't see it as a requirement. Developers can run these tests locally. You could add a pre-commit hook for that, given that the tests are fast, but that's not a requirement. I'd see these tests more as a tool that helps developers doing the QA that they would anyway have to do.

Some more background:

• I'm quite interested in using pa.
• Currently, pa feels not quite ready for productive use. There are some features that I'd like to have, and a few issues to iron out[^1][^2][^3].
• For the bugs, I'm considering to submit patches. It would be nice if these could come with tests that describe the new behavior and also make sure there are no regressions.

[^1]: pa edit should abort if the editor returns a non-zero exit code [^2]: pa edit should verify that the password is non-empty (EDIT: looks like that one actually just got fixed last week) [^3]: my number one feature request is a pa git command

[biox]

i like the idea of a testing suite, but i don't want to introduce any dependencies - neither gha nor shellspec. imo, we should just have a tests dir/file, and we could just write tests in there. shellspec just has too much esotericness for my liking

here's an example of what i mean.

this:

It "runs without parameters"
  When run script pa
  The output should include "a simple password manager"
  The status should be success
End

becomes:

./pa | grep -q "a simple password manager" ||
    die "pa should run without parameters"

i'd be willing to convert what you've written into this format, but it may take me awhile to get to it. if you wrote up something like that, i'd accept it! i am just very averse to growing our dependency list.

as for your feature requests~

pa edit should abort if the editor returns a non-zero exit code

does this patch work for your use-case?

diff --git a/pa b/pa
index 0dc9f83..99850fc 100755
--- a/pa
+++ b/pa
@@ -63,7 +63,8 @@ pw_edit() {
             die "Couldn't decrypt $name.age"
     }; fi

-    ${EDITOR:-vi} "$tmpfile"
+    ${EDITOR:-vi} "$tmpfile" ||
+        die "EDITOR exited non-zero"

     [ -s "$tmpfile" ] && {
         mkdir -p "$(dirname "./$name")" ||

my number one feature request is a pa git command

could you describe what you'd like to see here? @arcxio recently implemented auto-git support in https://github.com/biox/pa/pull/20

if you just want git subcommands, could you alias something up like this in your shell profile?

pa() {
    case $1 in
        g*)
            cd "${PA_DIR:=${XDG_DATA_HOME:=$HOME/.local/share}/pa/passwords}"
            shift
            git "$@"
        ;;

        *)
            command pa "$@"
        ;;
    esac
}

[Sjlver]

These are good points. A couple thoughts:

I agree with you that we want to keep dependencies minimal. That need not be the case for dev dependencies though, in my opinion. These are things that only developers need, but end-users don't. Dev dependencies already include things like shellcheck.

On the other hand, the tests are simple enough that we could work without any testing library. It might be a bit easier to make mistakes, and you don't get things like code coverage or pretty error messages. I'll aim to convert the tests. It might take two weeks or so, these are somewhat busy times.

does this patch work for your use-case?

Yes, I think that should do.

if you just want git subcommands, could you alias something up like this in your shell profile?

This, but built-in :)

I feel that an alias for this no longer makes sense, now that pa has a built-in git commit. Users should really be able to git push without having to care about pa internals, such as the location of the passwords folder. I think this is a core feature of a password manager, because backing up the passwords (via git push) is critical.

I'd happily trade other features against this one... there are features that I never use, like pa add. It's impossible to reliably auto-generate passwords, given that sites have wildly different policies around password length and character sets. I also typically want to add my username and other metadata to the password file. -- But I'm diverging :) Let me just say that git subcommands feel important to me.

[biox]

all dependencies carry weight, dev deps included - let's start with what we have already introduced (posix shell), and if that can't test as well as we need it to, let's consider alternatives at that point! i agree that testing is valid, but i think pa is small enough that shell will get the job done, plus the style can be similar to the style already used in pa itself.

Yes, I think that should do.

great! i've pushed the commit.

Users should really be able to git push without having to care about pa internals, such as the location of the passwords folder.

i agree, and in fact i have a little alias that i use to push my own passwords to remotes.

i suspect that introducing pa git <arbitrary git commands> isn't the right approach here - pa git push just carries too much weight - maybe we should have some specific ways of setting up a remote such that it gets pushed to during the auto-commit process? maybe we add PA_GIT_REMOTE= & if that env var is set, a remote push is attempted on every commit?

[arcxio]

do you have thoughts about this @arcxio?

they're similar to yours jes - the concept is cool, but I'm against using shellspec. additional concern I have is unclear maintenance status: the development seems to be ongoing right now but there were no releases in almost 4 years, and we could bump into bugs or broken backwards compatibility down the road (see shellspec/shellspec#216).

also these tests don't seem to work with dash as /bin/sh:

~/src/pa> ~/.local/bin/shellspec --shell dash

Finished in ? seconds (user ? seconds, sys ? seconds)
0 examples, 0 failures, aborted by an unexpected error

Aborted with status code [executor: 0] [reporter: 1] [error handler: 0]
Fatal error occurred, terminated with exit status 1.

and the error message is so non-descriptive I wouldn't even know how to approach fixing this.

I like the idea of a test suite without the framework.

Dev dependencies already include things like shellcheck.

shellcheck is not really a dev dependency, the actual dev dependency is a shell linter, in the same way "POSIX compliant shell" is a dependency: you can easily swap it with any other compatible program. say if shellcheck gets abandoned, it wouldn't be a problem because almost certainly there would be an alternative. I can't say the same would be possible with shellspec because the framework is so specific, instead of depending on POSIX sh it's more akin to depending on bash (if bash wasn't so widely used and well-maintained)

Users should really be able to git push without having to care about pa internals, such as the location of the passwords folder.

can you open an issue for that, so that we move this discussion there?

[Sjlver]

I've converted to first few tests to vanilla shellscripts. Please let me know what you think.

From the point of view of readability and clarity, I think it's a pretty clear change for the worse. We could probably improve this with some helper functions in a shared testing library?

[biox]

i took your input and produced a test script of the type that suits the project in https://github.com/biox/pa/pull/48

let's continue the convo over there - i'll close this PR for now!