-
Notifications
You must be signed in to change notification settings - Fork 3.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
CAT: improve test timeouts #31810
CAT: improve test timeouts #31810
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Current dependencies on/for this PR:
This comment was auto-generated by Graphite. |
b7c0857
to
d9e52b6
Compare
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'm going to assume that although running tests concurrently leads single tests to run slower we're still getting a net win out of it 😄
@@ -24,7 +24,7 @@ RUN poetry install --no-root --only main --no-interaction --no-ansi | |||
COPY . /app | |||
RUN poetry install --only main --no-cache --no-interaction --no-ansi | |||
|
|||
LABEL io.airbyte.version=2.0.2 | |||
LABEL io.airbyte.version=2.1.1 |
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.
😅
# Running tests in parallel can sometime delay the execution of the tests if downstream services are not able to handle the load. | ||
# This is why we set a timeout on tests that call command that should return quickly, like spec | ||
@pytest.mark.default_timeout(ONE_MINUTE) |
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.
Is it possible for us to know if we've set an option to run concurrently? I.e. set timeouts based on if we're running concurrently or not?
No worries if not, just a suggestion :)
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 know the pytest.xdist
exposes a worker_id
fixture that can help us check within the test execution if we're running concurrently or not. But these timeout are not controlled by the test themselves but at the pytest orcherstration level. If we want to set the timeout more dynamically I think we'd have to change something here where we tweak tests configuration and have access to fixture value. I think it might be early optimization at the moment. Let's consider this change if we observe more timeout-related problems.
@@ -517,7 +519,7 @@ def test_oauth_flow_parameters(self, actual_connector_spec: ConnectorSpecificati | |||
diff = paths_to_validate - set(get_expected_schema_structure(spec_schema)) | |||
assert diff == set(), f"Specified oauth fields are missed from spec schema: {diff}" | |||
|
|||
@pytest.mark.default_timeout(60) | |||
@pytest.mark.default_timeout(ONE_MINUTE) |
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.
Much clearer, thanks!
What
Closes #31809
Running test in parallel can sometime cause a increased load on the dagger-engine which can delay some test execution.
We've seen
TestDiscovery
timeout on source postgres (cf #31809)How
Testing
Running
airbyte-ci connecor --name=source-postgres test