-
Notifications
You must be signed in to change notification settings - Fork 42
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
chore: isolate execution of snowflake integration tests #2278
Conversation
@@ -356,3 +333,56 @@ jobs: | |||
# Fake GCS server with a sub-directory path; run with two different folder paths to assert no conflicts arise | |||
just slt -l gs://$TEST_BUCKET/path/to/folder/1 -o service_account_path=/tmp/fake-gcs-creds.json 'sqllogictests_native/*' | |||
just slt -l gs://$TEST_BUCKET/path/to/folder/2 -o service_account_path=/tmp/fake-gcs-creds.json 'sqllogictests_native/*' | |||
|
|||
service-integration-tests-snowflake: |
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 think we should just allow this one to fail.
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.
Then why even bother running the test? If you run a test that don't look at the output of and that can't fail, then it provides no value and we should just skip it entirely.
A few things:
- this is no longer in the required builders for tests.
- there's nothing that depends on this
- the changes to the concurrency settings (should) mean that only one of these tasks can run at once, which will (once all PRs will remediate the underlying cause of the failure (e.g. that too many tests are running at once, according to their error message, which agrees informally with what we see.)
I think we can add this in later, but I'd like to give this a week to settle in and see if it works in practice.
No description provided.