Skip to content
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 tests in python #2892

Merged
merged 16 commits into from
Mar 20, 2024

Conversation

hubertdeng123
Copy link
Member

@hubertdeng123 hubertdeng123 commented Mar 13, 2024

This adds a couple of exciting things and will be the first step into modernizing the test suite for self-hosted.

  1. integration tests in python
  2. pytest-sentry so that pytest failures will automatically be reported to a Sentry project
  3. retries for pytest test suite

@hubertdeng123 hubertdeng123 force-pushed the hubertdeng123/integration-tests-python branch from a824e7d to 1eadfea Compare March 14, 2024 21:35
@hubertdeng123 hubertdeng123 marked this pull request as ready for review March 14, 2024 22:46
@@ -15,12 +15,11 @@ export MINIMIZE_DOWNTIME=0

if [[ "$test_option" == "--initial-install" ]]; then
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The goal is to rewrite this file entirely in python using pytest, but baby steps!

cls.session = requests.Session()
response = None
run_shell_command("docker compose --ansi never up -d")
for i in range(TIMEOUT_SECONDS):
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is pretty ugly, open to suggestions on how to make polling for a response cleaner

Copy link
Member

@asottile-sentry asottile-sentry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a start at reviewing -- I think this is going in the right direction though! I suspect a lot will change from my initial comments so I didn't want to dig in too much into the rest of it

.github/workflows/test.yml Outdated Show resolved Hide resolved
_integration-test/custom-ca-roots/setup.sh Outdated Show resolved Hide resolved
_integration-test/run.py Outdated Show resolved Hide resolved
_integration-test/run.py Outdated Show resolved Hide resolved
@hubertdeng123 hubertdeng123 force-pushed the hubertdeng123/integration-tests-python branch from 03d714b to bf0905b Compare March 15, 2024 20:25
Copy link
Member

@asottile-sentry asottile-sentry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the shell=True is a blocker

_integration-test/run.py Outdated Show resolved Hide resolved
import subprocess
import os
from functools import lru_cache
import requests
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would not recommend using requests in new code

if you can get away with it, stdlib urllib.request does not require additional dependencies. httpx is a modern replacement if you find urllib.request too cumbersome

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

switched to using httpx, since I wanted a good way to save cookies

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reason why requests is not the preferred choice?

_integration-test/run.py Outdated Show resolved Hide resolved
_integration-test/run.py Outdated Show resolved Hide resolved
_integration-test/run.py Outdated Show resolved Hide resolved
_integration-test/run.py Outdated Show resolved Hide resolved
_integration-test/run.py Outdated Show resolved Hide resolved
_integration-test/run.py Outdated Show resolved Hide resolved
_integration-test/run.py Outdated Show resolved Hide resolved
response = client.get(
request, follow_redirects=True, headers={"Referer": SENTRY_TEST_HOST}
)
if response.status_code == 200:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function won't work for spans as it's waiting for a span to be ingested to succeed, so it's receiving a 200 no matter what and looking for the data field to not be empty.

We risk flaky tests if we leave it at that. Maybe we can have a custom behavior via a function to return True/False if it's considered a success of if we need to retry?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, thanks for the context. I added a optional validator function passed into poll_for_response that checks the response json to check to ensure the data field length is greater than 0

@@ -15,7 +15,7 @@ export MINIMIZE_DOWNTIME=0

if [[ "$test_option" == "--initial-install" ]]; then
echo "Testing initial install"
source _integration-test/run.sh
pytest --reruns 5 _integration-test/run.py
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that even needed? If we have proper retries when polling data, we likely don't need to retry the tests up to 5 times, do we?

Copy link
Member

@asottile-sentry asottile-sentry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

neat! probably close to shippable :D

_integration-test/run.py Outdated Show resolved Hide resolved
_integration-test/run.py Outdated Show resolved Hide resolved
_integration-test/run.py Outdated Show resolved Hide resolved
_integration-test/run.py Outdated Show resolved Hide resolved
@@ -15,7 +15,7 @@ export MINIMIZE_DOWNTIME=0

if [[ "$test_option" == "--initial-install" ]]; then
echo "Testing initial install"
source _integration-test/run.sh
pytest --reruns 5 _integration-test/run.py
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would recommend not having reruns -- they're a crutch that allows flakiness to creep in and ideally we wouldn't have them in sentry

Copy link
Member Author

@hubertdeng123 hubertdeng123 Mar 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Primary reason behind this is because docker compose itself is quite flaky in CI, and I wanted to have a way to rerun the fixture that brings self-hosted up and report the flakiness

getsentry/team-ospo#242

@hubertdeng123 hubertdeng123 force-pushed the hubertdeng123/integration-tests-python branch from 935a84b to d20b057 Compare March 19, 2024 17:59
Copy link
Member

@asottile-sentry asottile-sentry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hubertdeng123 hubertdeng123 merged commit b3d3ce0 into master Mar 20, 2024
10 checks passed
@hubertdeng123 hubertdeng123 deleted the hubertdeng123/integration-tests-python branch March 20, 2024 19:27
@github-actions github-actions bot locked and limited conversation to collaborators Apr 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants