Test improvements#73
Merged
Merged
Conversation
theyoyojo
reviewed
Apr 1, 2024
theyoyojo
reviewed
Apr 1, 2024
theyoyojo
reviewed
Apr 1, 2024
Contributor
|
Tests fail when test directory (not in source control) doen't exist: |
c9614dc to
7b5f7ab
Compare
Fixes: 9008715 ("test: add initial basic testing script")
Fixes: 0cfe1f2 ("test: use shellcheck to scan the two scripts in the repo")
If there are syntax or style errors, we should bail early instead of failing in potentially a more complicated way by running the real functionality tests.
instead of introducing a function to print the command being executed and exit the shell if it fails, just use the builtin set -e and set -x flags in the shell.
we don't need anything bash specific so use a more generic shell that is going to be present on all systems
this script calls shellcheck on all the .sh files similar to test-style.sh from orbit.
Shellcheck SC2086 suggests doing this. It is unlikely that the container name could have a space (is it even possible?) but it is good practice nonetheless and tidier than having a nolint comment.
we now lint the warpdrive script during the tests.
Shellcheck SC3040 warns about using pipefail when the shebang is /bin/sh since it is not specified by posix. There are no commands with pipes in the script so it wasn't doing anything anyways and can just be removed to make shellcheck happy.
we now lint create_dev_keys.sh during the tests
Since podman is never directly invoked in test.sh, it is clearer to delegate checking for it to the script that actually uses it (warpdrive)
The inclusion of chcon was a relic of an earlier version of the test script that never even got merged. That version would fully delete the email directory and so it needed to use chcon to set it back to container_file_t before starting the tests, but during the review process the code was changed to just delete all emails and logs within the folder instead of deleting the folder itself, so this code was and is no longer necessary. Fixes: 9008715 ("test: add initial basic testing script")
flake8 is never invoked directly in test.sh so it is kind of confusing that it is listed as a requirement. We can just delegate checking for required commands to any subscripts instead of having to bubble the dependencies up so it is easier to keep them up to date.
the test script now maintains a stack of functions to run at exit from the shell. the `add_cleanup` function pushes a new entry to the stack and then regardless of how the script terminates (error or successfully) any items that have been pushed on to the stack are executed in reverse order. As an example, the user deletion code is moved from being executed after all the authentication related test code runs, to being pushed on to the cleanup stack immediately after the user creation is performed so that regardless of whether the subsequent tests pass, the user will be deleted and the script can be run idempotently.
7b5f7ab to
dd3cb32
Compare
instead of deleting and recreating the folder, just rm the contents
Even if the files in the test folder are owned by root, as long as the folder is owned by the regular user, they can be deleted. Now that the folder is not recreated every time the script is run it can just stick around as a folder owned by the unprivileged user even when it is used to hold the output of runs using sudo.
since every curl command has --verbose --insecure --fail --no-progress-meter they can be stored in a variable and referenced where used
insecure disables all cert verification including basic things like the expiry date matching hostnames, etc. Instead we can provide the cert public key from the ssl folder to curl so that it can recognize the cert even though it is self signed. Fixes #56
dd3cb32 to
012c04f
Compare
theyoyojo
approved these changes
Apr 1, 2024
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
substantial cleanup / refactor of tests to make them more useful.
Fixes: #56