Add tests for passwdqc pwqcheck

[Zaiba-S]

Currently, there are no tests implemented for the package, so this serves as an initial attempt to implement basic testing coverage. This PR introduces an initial set of tests for the pwqcheck binary of the passwdqc package.

Any feedback or suggestions would be appreciated.

[solardiz]

My thinking was to start with what I've been doing in manual testing - running pwqcheck on a large file and confirming the hash of its output isn't unexpectedly changed. This wouldn't directly test the individual checks, but it would ensure stable behavior. So if we ever make code changes that change the behavior on such large test input, we'll need to explicitly acknowledge this and update the hash. A drawback is the large input file would need to be stored somewhere in here or download from somewhere external (e.g., the run/password.lst file we have in the john repo) or reproducibly generated.

$ LD_LIBRARY_PATH=. ./pwqcheck -1 --multi < ~/john/run/password.lst | grep -c ^OK:
1733
$ LD_LIBRARY_PATH=. ./pwqcheck -1 --multi < ~/john/run/password.lst | grep -c ^Bad
1793975
$ LD_LIBRARY_PATH=. ./pwqcheck -1 --multi < ~/john/run/password.lst | md5sum 
3eccb6ef49eb84b494c61954c240e76c  -

I see that you also test a few features that the above would not.

[Zaiba-S]

My thinking was to start with what I've been doing in manual testing - running pwqcheck on a large file and confirming the hash of its output isn't unexpectedly changed. This wouldn't directly test the individual checks, but it would ensure stable behavior. So if we ever make code changes that change the behavior on such large test input, we'll need to explicitly acknowledge this and update the hash. A drawback is the large input file would need to be stored somewhere in here or download from somewhere external (e.g., the run/password.lst file we have in the john repo) or reproducibly generated.

$ LD_LIBRARY_PATH=. ./pwqcheck -1 --multi < ~/john/run/password.lst | grep -c ^OK:
1733
$ LD_LIBRARY_PATH=. ./pwqcheck -1 --multi < ~/john/run/password.lst | grep -c ^Bad
1793975
$ LD_LIBRARY_PATH=. ./pwqcheck -1 --multi < ~/john/run/password.lst | md5sum 
3eccb6ef49eb84b494c61954c240e76c  -

I see that you also test a few features that the above would not.

Thanks for sharing your approach Your method of hashing the output of pwqcheck on a large input file sounds like a great way to ensure stability across changes. Are you suggesting that we implement this approach in addition to the individual tests I proposed? If so, For storing the large input file and expected output hash for comparison, do you have a preference on where it should be stored ?

[solardiz]

This PR currently fails our whitespace-errors test:

Run git diff-index --check --cached 4b825dc642cb6eb9a060e54bf8d69288fbee4904
tests/test-pwqcheck-length.sh:14: trailing whitespace.
+    
tests/test-pwqcheck-length.sh:18: trailing whitespace.
+    
tests/test-pwqcheck-length.sh:22: trailing whitespace.
+    
tests/test-pwqcheck-multi.sh:13: trailing whitespace.
+    
tests/test-pwqcheck-multi.sh:16: trailing whitespace.
+    
tests/test-pwqcheck-multi.sh:25: trailing whitespace.
+    
tests/test-pwqcheck-multi.sh:39: trailing whitespace.
+    
tests/test-pwqcheck-multi.sh:85: trailing whitespace.
+  
tests/test-pwqcheck-multi.sh:109: trailing whitespace.
+            echo "StrongP@ss${i}!" 
tests/test-pwqcheck-multi.sh:111: trailing whitespace.
+            echo "weak${i}" 
tests/test-pwqcheck-similarity.sh:94: new blank line at EOF.

[solardiz]

Are you suggesting that we implement this approach in addition to the individual tests I proposed?

In general, yes. In this same PR, no.

If so, For storing the large input file and expected output hash for comparison, do you have a preference on where it should be stored ?

Currently no. I'd like to hear from @ldv-alt on this. Off the top of my head, the options are:

1. In this repo and included in release archive (so that make check can use it from there as well).
2. In this repo, but excluded from release archive (available for use in GitHub Actions).
3. External reference (what kind?) to john/run/password.lst.
4. Generate a large list into a pipe programmatically (the lines wouldn't look so much like real passwords, but that's OK if we're only testing stability).

[solardiz]

The color output could be undesired when redirected to a build log file. I suggest to use color only when outputting to a tty, which I think you can test with test -t 1, e.g. if [ -t 1 ]; then.

We use 8-wide tabs for indentation in this source tree, and I think it makes sense to continue with this style for the shell scripts in here as well (not switch to 4 spaces).

[solardiz]

Please also add your copyright statements and references to LICENSE as source code comments near the start of your scripts, similarly to how we have this in existing source files.

[solardiz]

This PR currently fails our whitespace-errors test:

Run git diff-index --check --cached 4b825dc642cb6eb9a060e54bf8d69288fbee4904

This is still failing. You can run the above git command on your computer to see (and then fix) these errors before you push.

[AdithyaTSIP]

Hi @solardiz

Hope this message finds you well.

Any chance of this PR getting merged if no additional feedback?

Also there are 2 more PR's (#42 and #43 ) that adds a few more tests. Just curious to understand if the project is planning a different approach to the addition of tests.

[ldv-alt]

I'll have a look.

[solardiz]

Hi. Sorry for the lack of progress here. I haven't forgotten, and intend to revisit. Meanwhile, any comments from @ldv-alt would help as well.

[solardiz]

Thank you @Zaiba-S! As you noticed, I merged your 3 PRs a few hours ago, with intent to complete my review and testing and apply further changes shortly. I did that now - and it did take me several hours. There's still much room for improvement, but the tests are now run in GitHub Actions. Thanks again.