Skip to content
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

change(tests): Adds timeouts tonic clients in tests #8724

Merged
merged 1 commit into from
Jul 27, 2024
Merged

Conversation

arya2
Copy link
Contributor

@arya2 arya2 commented Jul 26, 2024

Motivation

The test_grpc_methods_mocked seemed to hang in CI here: https://github.com/ZcashFoundation/zebra/actions/runs/10101606903/job/27935542667?pr=8694#step:12:2915

Tonic doesn't add any timeout by default on the server or the client, the test isn't checking for panics in the spawned tokio tasks for handling mock service responses, and the mock service isn't wrapped in a Timeout tower layer.

Solution

  • Adds a 2s timeout to the tonic client endpoints in the affected tests
  • Increases the timeout of the relevant mock services from the 300ms default to 2s

PR Author's Checklist

  • The PR name will make sense to users.
  • The PR provides a CHANGELOG summary.
  • The solution is tested.
  • The documentation is up to date.
  • The PR has a priority label.

PR Reviewer's Checklist

  • The PR Author's checklist is complete.
  • The PR resolves the issue.

@arya2 arya2 added C-cleanup Category: This is a cleanup C-testing Category: These are tests C-tech-debt Category: Code maintainability issues P-Low ❄️ labels Jul 26, 2024
@arya2 arya2 self-assigned this Jul 26, 2024
@arya2 arya2 requested a review from a team as a code owner July 26, 2024 00:08
@arya2 arya2 requested review from upbqdn and removed request for a team July 26, 2024 00:08
@github-actions github-actions bot added the C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG label Jul 26, 2024
oxarbitrage
oxarbitrage previously approved these changes Jul 26, 2024
Copy link
Contributor

@oxarbitrage oxarbitrage left a comment

Choose a reason for hiding this comment

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

Looks good.

Base automatically changed from fix-new-clippy-lints to main July 26, 2024 14:29
@mergify mergify bot dismissed oxarbitrage’s stale review July 26, 2024 14:29

The base branch was changed.

@mergify mergify bot requested a review from a team as a code owner July 26, 2024 14:29
@arya2 arya2 requested a review from oxarbitrage July 26, 2024 20:57
@mergify mergify bot merged commit 14f473d into main Jul 27, 2024
135 checks passed
@mergify mergify bot deleted the add-test-timeouts branch July 27, 2024 15:20
@arya2 arya2 mentioned this pull request Aug 1, 2024
43 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-cleanup Category: This is a cleanup C-tech-debt Category: Code maintainability issues C-testing Category: These are tests C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG P-Low ❄️
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants