-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Integration test improvements #2858
Changes from 5 commits
02ccedd
07c059e
364d0d2
9dd9ac0
c21576f
4308b2c
b22f1eb
9c4fbfc
ffabc9f
2ced8fb
7559e55
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -59,6 +59,8 @@ jobs: | |
strategy: | ||
fail-fast: false | ||
matrix: | ||
test_type: ["initial-install", "customizations"] | ||
compose_version: ["v2.0.1", "v2.7.0"] | ||
include: | ||
- compose_version: "v2.0.1" | ||
compose_path: "/usr/local/lib/docker/cli-plugins" | ||
|
@@ -82,8 +84,23 @@ jobs: | |
sudo curl -L https://github.com/docker/compose/releases/download/${{ matrix.compose_version }}/docker-compose-`uname -s`-`uname -m` -o "${{ matrix.compose_path }}/docker-compose" | ||
sudo chmod +x "${{ matrix.compose_path }}/docker-compose" | ||
|
||
- name: Install self-hosted | ||
uses: nick-fields/retry@v3 | ||
with: | ||
max_attempts: 3 | ||
timeout_minutes: 10 | ||
retry_on: error | ||
command: | | ||
export REPORT_SELF_HOSTED_ISSUES=0 | ||
./install.sh | ||
|
||
- name: Integration Test | ||
run: ./integration-test.sh | ||
uses: nick-fields/retry@v3 | ||
with: | ||
max_attempts: 3 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just so I understand: what was the behavior before you added these settings? Unlimited retries? No timeout so it hung until the action crashed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The tests would fail from flakes way too often, adding this in drastically increases the chance the tests pass when they should There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So, in effect, the previous (implicit) setting was There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, that's correct. The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, that's fine. I just wanted to understand the change. LGTM! |
||
timeout_minutes: 25 | ||
retry_on: error | ||
command: ./integration-test.sh --${{ matrix.test-group }} | ||
|
||
- name: Inspect failure | ||
if: failure() | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,7 +28,12 @@ teardown() { | |
DID_TEAR_DOWN=1 | ||
|
||
if [ "$1" != "EXIT" ]; then | ||
echo "An error occurred, caught SIG$1 on line $2" | ||
set +x | ||
error_msg="An error occurred, caught SIG$1 on line $2" | ||
echo "$error_msg" | ||
dsn="https://[email protected]/6627632" | ||
local sentry_cli="docker run --rm -v $(pwd):/work -e SENTRY_DSN=$dsn getsentry/sentry-cli" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Odd to me that we set REPORT_ISSUES=0 above, but then send an envelope here anyway? Maybe I am misunderstanding something, but why would we not just do There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, I can incorporate this into the existing logic for |
||
$sentry_cli send-event -m "$error_msg" --logfile $log_file | ||
fi | ||
|
||
echo "Tearing down ..." | ||
|
@@ -41,10 +46,10 @@ echo "${_endgroup}" | |
echo "${_group}Starting Sentry for tests ..." | ||
# Disable beacon for e2e tests | ||
echo 'SENTRY_BEACON=False' >>$SENTRY_CONFIG_PY | ||
echo y | $dcr web createuser --force-update --superuser --email $TEST_USER --password $TEST_PASS | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. docker run is slower than just an exec into a running container There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is but it is also intentional to keep the run separate from the main web process. That said for a one-off thing like this, I think using |
||
$dc up -d | ||
printf "Waiting for Sentry to be up" | ||
timeout 90 bash -c 'until $(curl -Isf -o /dev/null $SENTRY_TEST_HOST); do printf '.'; sleep 0.5; done' | ||
echo y | $dc exec web sentry createuser --force-update --superuser --email $TEST_USER --password $TEST_PASS | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What happens if we exec into the web container but it isn't ready yet? Does docker wait until it is ready, or does this fail? If it's the former, we should There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Docker compose up will only succeed if the container healthcheck for web passes, so I don't think this will be a problem. That is performed on a previous line, so when the tests get to the createuser logic the web container will always be ready |
||
printf "Waiting for Sentry to be up" | ||
echo "" | ||
echo "${_endgroup}" | ||
|
||
|
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.
Any chance this could actually increase the flake rate, due to timeouts becoming more frequent? Or are flakes caused by the actions itself timing out?
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.
The typical install logic takes around 4-5 minutes. I doubled that time for the timeout so I don't think this should ever increase the flake rate.