Conversation
This makes sense, and also speeds up the execution of the test code.
The Docker composition is based on that in cisagov/guacamole-composition but is altered to allow an instance of guacscanner running on the Docker host to access the mock AWS IMDS service and the PostgreSQL instance. Note that we also have to add a library to the test requirements of the Python library and set up some test fixtures. We also have to pre-populate some dummy secrets used by the Docker composition.
This is preferable to specifying them manually.
I will uncomment these tests and adapt them to the new testing Docker composition one by one.
This is just in case the Docker host has an external IP.
Moto mocks AWS services.
Also use pytest-env to set an environment variable necessary for moto to make use of the server we are now running in the Docker composition; normally moto starts a local server on the host where pytest is being run.
With alterations for new Docker composition test environment.
github-actions
Bot
added
version bump
Instead just run Moto as a local library in the pytest run.
This isn't needed in this repository.
This container is unnecessary here as we have no actual instances to connect to via guacamole.
This container doesn't bind to port 80, so there is no need fro it to have this capability.
Why mix and match?
We want these to be class scope so that they remain up for related tests in the same class but the PostgreSQL database gets destroyed between test classes.
This will allow the PostgreSQL container to prepare itself for use before it is queried by any test code.
This allows boto3 to function with Moto (our mock AWS library).
github-actions
Bot
added
the
github-actions
Using a scope of function here makes the PostgreSQL container have the same scope as the Moto mock AWS library. The latter resets itself after every test function. Also remove a couple of unused fixtures and revert the scope of the Docker composition itself to session.
I don't think this is necessary, but it certainly doesn't hurt.
This is the proper place for this code.
This should ensure that port 5432 is free on the GitHub runner. The fact that the PostgreSQL service was running on the GitHub runner was previously causing issues when running pytest (with the Docker composition) in GitHub Actions.
Also create a dummy (empty) VPC to search instead of using a dummy VPC ID.
There are currently various issues with running Docker on the GitHub-hosted Mac runners. I created #274 to remind us to support Mac when that becomes possible. The official Guacamole images currently only support x86_64 and ARM64 on Linux; therefore, we cannot currently run the tests on Windows. I created #273 to remind us to again support Windows when that becomes possible.
This is a better home for these testing-specific files.
This is a better location for this testing-specific file.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR reworks the test strategy to run against a real PostgreSQL instance in Docker instead of heavily mocking psycopg, aligning test behavior more closely with production usage of guacscanner.
Changes:
- Replaced most database-mocking tests with Docker-backed integration tests using
python-on-whales. - Added Docker Compose and secret fixtures for a PostgreSQL/Guacamole test environment.
- Updated the CLI to support overriding the PostgreSQL hostname and trimmed newline characters when reading secrets from files.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
tests/test_guacscanner.py |
Reorganized tests into classes and converted many cases to Docker-backed integration tests. |
tests/secrets/postgres-username |
Added test secret for the PostgreSQL username. |
tests/secrets/postgres-password |
Added test secret for the PostgreSQL password. |
tests/conftest.py |
Added pytest fixtures to start/stop the Docker composition and expose Postgres details. |
tests/compose.yml |
Added Docker Compose stack for PostgreSQL and Guacamole-based test setup. |
src/guacscanner/guacscanner.py |
Added --postgres-hostname, stripped file-based secret values, and adjusted loop timing. |
src/guacscanner/_version.py |
Bumped version to 3.0.2-rc.1. |
pyproject.toml |
Added python-on-whales to test dependencies. |
.github/workflows/build.yml |
Updated test matrices and CI setup for Docker-backed tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+16
to
+26
| LOG_LEVELS: list[str] = [] | ||
| if sys.version_info >= (3, 11): | ||
| LOG_LEVELS = [*logging.getLevelNamesMapping()] | ||
| else: | ||
| # The logging.getLevelNamesMapping method was only introduced in | ||
| # Python 3.11. | ||
| LOG_LEVELS = [ | ||
| logging.getLevelName(x) | ||
| for x in range(0, 101) | ||
| if not logging.getLevelName(x).startswith("Level") | ||
| ] |
Comment on lines
+69
to
+72
| volumes: | ||
| - read_only: true | ||
| source: initdb | ||
| target: /opt/guacamole/extensions/guacamole-auth-jdbc/postgresql/schema |
Member
Author
There was a problem hiding this comment.
This appears to work, and I followed the official instructions from Guacamole.
Comment on lines
+26
to
+27
| ports: | ||
| - 127.0.0.1:5432:5432/tcp |
Member
Author
There was a problem hiding this comment.
This is OK. We want our PostgreSQL to be listening on localhost:5432 for testing purposes.
| run: sudo systemctl stop postgresql | ||
| - name: Run tests | ||
| env: | ||
| RELEASE_TAG: ${{ github.event.release.tag_name }} |
We can't call another member function here since that will reset Moto (the mock AWS library we are using). Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
This gets rid of some errors from our flake8 pre-commit linter.
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.
🗣 Description
This pull request reworks the test code to use a Docker composition running an actual PostgreSQL instance. As
pytestruns the tests interact with the containers in the Docker composition instead of making extensive use of mock objects.💭 Motivation and context
These changes make testing run faster and provide for better tests. This is also the way that this code is intended to be run anyway in cisagov/guacamole-composition.
Resolves #270.
🧪 Testing
All automated tests pass.
✅ Pre-approval checklist
bump_versionscript if this repository is versioned and the changes in this PR warrant a version bump.✅ Pre-merge checklist