-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add integration test framework #492
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very interesting PR. Just my just pass. I'll come back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one more minor comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice PR!
if [ "${option}" = "no-suffix" ]; then | ||
configs_cmd_suffix= | ||
fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If $(print_config_command)
was used, wouldn't if [ -z "$1" ]; then
work the same?
Just curious. no-suffix
is fine, but seems like a roundabout way with unnecessary hardcoded values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By using a constant like 'no-suffix', it allows us to easily add other options in the future as well. For this specific use case [ -z "$1" ]
would work as well but it's not as friendly to future developers.
I also like no-suffix
because it's self-documenting 🤷
@@ -22,3 +22,10 @@ services: | |||
REQUESTS_CA_BUNDLE: /etc/ssl/certs/ca-certificates.crt | |||
restart: always | |||
logging: *default-logging | |||
healthcheck: | |||
test: ["CMD", "curl", "--fail", "${BIRDHOUSE_FQDN_PUBLIC}"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally we should use an internal endpoint/port to avoid loop-back requests.
Also, consider using another endpoint such as the /version
, since the root path gets redirected to whichever "frontend" entrypoint is defined, which might be inconsistent or pinging another redirected service from the stack.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea to an internal endpoint.
Just a note: curl won't follow redirects unless the --location
flag is used so we don't have to worry about redirects to inconsistent places in this case.
So something like curl --fail http://localhost
is sufficient because either it will redirect (success) if the nginx component is working or will fail if it isn't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than the minor NO_COLOR left, great PR !
|
||
|
||
@pytest.fixture(scope="module", autouse=True) | ||
def start_stack(request, cli_path, local_env_file, tmp_data_persist_root, stack_info, pytestconfig): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't seen usage of this new fixture. In fact, same for most of the new fixtures here.
I am guessing you are simply setting up for the future?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixtures with autouse=True
are used automatically in tests within their scope. So you won't see most of these referenced explicitly in tests (but they are being used!).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OH ! Interesting!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just needs a line to changelog about the deprecated wps-healthchecks.
Overview
This update adds a framework for testing the deployed stack using pytest. This will allow developers to check
that their changes are consistent with the existing stack and to add additionally tests when new functionality
is introduced.
Changes to implement this include:
tests/unit/
directorytests/integration/
directory. More tests will be added in thefuture!
conftest.py
scripts updated to bring the stack up/down in a consistent way for the integration tests.birdhouse
interface script updated to support testing infrastructure (this should not change anything forother end-users).
birdhouse
interface to improve user experience.without the use of the
canarie-api
component.optional-components/wps-healthchecks
component since the healthchecks are now added by defaultto all WPS components.
Next steps:
Changes
Non-breaking changes
Breaking changes
Related Issue / Discussion
Additional Information
CI Operations
birdhouse_daccs_configs_branch: master
birdhouse_skip_ci: false