-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Write Customization tests in python #2918
Write Customization tests in python #2918
Conversation
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
11f9bea
to
5c2fd16
Compare
c93b5e5
to
2ccd1a5
Compare
bcdccaa
to
82eed1e
Compare
a4f67e4
to
1799f09
Compare
.github/workflows/test.yml
Outdated
@@ -16,7 +16,7 @@ defaults: | |||
jobs: | |||
e2e-test: | |||
if: github.repository_owner == 'getsentry' | |||
runs-on: ubuntu-20.04 | |||
runs-on: ubuntu-22.04 |
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.
bumped this while at it
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.
nit: Imo we should split this into a separate PR, since I could imagine cases where we want to only roll this bump back.
### pytest-sentry configuration ### | ||
if [ "$GITHUB_REPOSITORY" = "getsentry/self-hosted" ]; then | ||
echo "PYTEST_SENTRY_DSN=${{ env.SELF_HOSTED_TESTING_DSN }}" >> $GITHUB_ENV | ||
echo "PYTEST_SENTRY_DSN=$SELF_HOSTED_TESTING_DSN" >> $GITHUB_ENV |
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.
This was previously not being picked up correctly
time.sleep(1) | ||
else: | ||
if response.status_code == 200: | ||
break | ||
else: | ||
raise AssertionError("timeout waiting for self-hosted to come up") | ||
|
||
if request.config.getoption("--customizations") == "True": | ||
os.environ['TEST_CUSTOMIZATIONS'] = "True" | ||
script_content = '''\ |
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.
These lines are definitely covered by testing, not sure why Codecov is complaining here.
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.
That's the reason why Test / integration test (True, v2.0.1)
and Test / integration test (True, v2.7.0)
are taking 8 minutes longer
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.
nit: It's pretty hard for a naive observer to figure out what the "True" in these variants means. Maybe we could have values be Customization Enabled
vs Customization Disabled
rather than True
/False
?
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.
Apparently, you can create a custom name for the job. Now it looks like
Test / integration test v2.0.1 - customizations disabled
.github/workflows/test.yml
Outdated
@@ -16,7 +16,7 @@ defaults: | |||
jobs: | |||
e2e-test: | |||
if: github.repository_owner == 'getsentry' | |||
runs-on: ubuntu-20.04 | |||
runs-on: ubuntu-22.04 |
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.
nit: Imo we should split this into a separate PR, since I could imagine cases where we want to only roll this bump back.
time.sleep(1) | ||
else: | ||
if response.status_code == 200: | ||
break | ||
else: | ||
raise AssertionError("timeout waiting for self-hosted to come up") | ||
|
||
if request.config.getoption("--customizations") == "True": | ||
os.environ['TEST_CUSTOMIZATIONS'] = "True" | ||
script_content = '''\ |
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.
nit: It's pretty hard for a naive observer to figure out what the "True" in these variants means. Maybe we could have values be Customization Enabled
vs Customization Disabled
rather than True
/False
?
for command in commands: | ||
result = subprocess.run(command, check=False) | ||
if os.getenv("TEST_CUSTOMIZATIONS", "False") == "True": | ||
assert result.returncode == 0 |
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.
Does test_customization
run in both matrix variants?
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.
Yes, it does. If customizations enabled, it checks to ensure that they are truly enabled by making sure a dependency can be imported that is included in the custom image (ldap). If not, it checks to ensure that an error is thrown when trying to import a dependency that comes from the custom image.
This PR adds a few things