Skip to content

Added retries to the container state check in TestDocker #12640

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
May 20, 2025
Merged

Added retries to the container state check in TestDocker #12640

merged 1 commit into from
May 20, 2025

Conversation

k-a-il
Copy link
Contributor

@k-a-il k-a-il commented May 19, 2025 •
edited
Loading

Motivation

I missed fixing one of the flaky tests in the TestDockerClient class in the PR #12625 that was merged today. Like test_is_container_running, test test_running_container_names fails due to a race condition:

FAILED tests/integration/docker_utils/test_docker.py::TestDockerClient::test_running_container_names[SdkDockerClient] - AssertionError: assert 'lst_test_4337619e' not in ['lst_test_4337619e', 'sleepy_tu']

We need to wait until the container is fully stopped. I also added a retry to the container start step, just to be safe.

Changes

  • Added function _assert_container_state which is used in test_running_container_names and test_is_container_running
  • Added retries to test_running_container_names

Testing

Tested here (transcribe tests are failing but it is not connected to changes in this PR, tests changed in this PR are passing): https://github.com/localstack/localstack/actions/runs/15117166475

@k-a-il k-a-il requested review from silv-io and dfangl May 19, 2025 15:33
@k-a-il k-a-il self-assigned this May 19, 2025
@k-a-il k-a-il added the semver: patch Non-breaking changes which can be included in patch releases label May 19, 2025
@github-actions GitHub Actions
Copy link

LocalStack Community integration with Pro

2 files  ±    0  2 suites  ±0   32s ⏱️ - 1h 44m 42s
2 tests  - 4 450  0 ✅  - 4 067  2 💤  - 383  0 ❌ ±0 
4 runs   - 4 450  0 ✅  - 4 067  4 💤  - 383  0 ❌ ±0 

Results for commit d5415dc. ± Comparison against base commit fa0bbcd.

This pull request removes 4450 tests. 
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_lambda_dynamodb
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_opensearch_crud
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_search_books
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_setup
tests.aws.scenario.kinesis_firehose.test_kinesis_firehose.TestKinesisFirehoseScenario ‑ test_kinesis_firehose_s3
tests.aws.scenario.lambda_destination.test_lambda_destination_scenario.TestLambdaDestinationScenario ‑ test_destination_sns
tests.aws.scenario.lambda_destination.test_lambda_destination_scenario.TestLambdaDestinationScenario ‑ test_infra
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_prefill_dynamodb_table
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input0-SUCCEEDED]
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input1-SUCCEEDED]
…

@k-a-il k-a-il marked this pull request as ready for review May 20, 2025 10:52
silv-io
silv-io approved these changes May 20, 2025
View reviewed changes
Copy link
Member

@silv-io silv-io left a comment

Choose a reason for hiding this comment

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

LGTM!

tests/integration/docker_utils/test_docker.py
@@ -1273,22 +1277,39 @@ def test_run_container_non_existent_image(self, docker_client: ContainerClient):
def test_running_container_names(self, docker_client: ContainerClient, dummy_container):
docker_client.start_container(dummy_container.container_id)
name = dummy_container.container_name
assert name in docker_client.get_running_container_names()
retry(
Copy link
Member

Choose a reason for hiding this comment

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

comment: This works because the retry catches the AssertionError, which doesn't happen in, e.g., poll_condition. Was a bit confused at first :D

dfangl
dfangl approved these changes May 20, 2025
View reviewed changes
Copy link
Member

@dfangl dfangl left a comment

Choose a reason for hiding this comment

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

LGTM!

@k-a-il k-a-il merged commit 793d96f into master May 20, 2025
73 of 75 checks passed
@k-a-il k-a-il deleted the circleci-to-gh-fix-test branch May 20, 2025 12:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@silv-io silv-io silv-io approved these changes

@dfangl dfangl dfangl approved these changes

Assignees

@k-a-il k-a-il

Labels
semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@k-a-il @silv-io @dfangl