Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to reduce integration test runtime by removing an apparent duplicated query operation in the materialized view update test, helping prevent CI from hitting long timeouts. It also makes a Docker repository integration test use unique names to avoid potential collisions.
Changes:
- Removed a duplicated
sleep+query_asyncblock intest_materialized_view_reflects_table_updatesto reduce test runtime. - Updated
test_get_docker_repo_by_idto generate uniquerepository_nameandnamevalues via a short UUID suffix.
Reviewed changes
Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
tests/integration/synapseclient/operations/async/test_factory_operations_async.py |
Adds a UUID suffix to Docker repository test names to reduce collision risk. |
tests/integration/synapseclient/models/async/test_materializedview_async.py |
Removes a duplicated query operation in a materialized view integration test to speed up execution. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
thomasyu888
left a comment
There was a problem hiding this comment.
Thanks for putting this up and I'll defer to @BryanFauble for a review of this as I added a comment that requires some discussion.
|
|
||
| # AND querying again (with delay for eventual consistency) | ||
| await asyncio.sleep(2) | ||
| query_result = await materialized_view.query_async( |
There was a problem hiding this comment.
There is probably a reason this is run twice, because synapse is eventually consistent. So the first query will cause the view to be indexed which will then return the results at the next step.
This is part of how Synapse works, doing the 2 second sleep isn't enough of a safe guard.
There was a problem hiding this comment.
What I think we would need to do across the board for all eventually consistent calls is to have a backoff and retry mechanism.
In the integration tests on the Java backend we do just that. Wait until some condition we want appears, then continue. If we do that then tests wouldn't be constantly rerunning.
There was a problem hiding this comment.
In Schematic, we used Tenacity for this, something like:
from tenacity import retry, stop_after_attempt, wait_exponential_jitter
@retry(
# Stop retrying completely after 5 attempts
stop=stop_after_attempt(5),
wait=wait_exponential_jitter(multiplier=1, max=10, jitter=1)
)
Perhaps we should try that here for now?
There was a problem hiding this comment.
That is a great idea @andrewelamb, we would need to wrap the logic that is checking "If we can proceed to the conditions being asserted" that way a legitimate failing assertion doesn't retry the entire setup/test once it's run out of retry budget.
There was a problem hiding this comment.
Thanks for the discussion @andrewelamb and @BryanFauble . I created a ticket here: https://sagebionetworks.jira.com/browse/SYNPY-1901
Problem:
test_materialized_view_reflects_table_updateswas taking a long time to run, and the job here hit 3 hours time limit.Solution:
Remove the duplicated code block that I found