Skip to content

consistent error class ServerError, test suite#744

Open
davidzhao wants to merge 1 commit into
mainfrom
dz/api-tests
Open

consistent error class ServerError, test suite#744
davidzhao wants to merge 1 commit into
mainfrom
dz/api-tests

Conversation

@davidzhao

Copy link
Copy Markdown
Member

ServerError is now the error base class. we've also kept TwirpError as an alias.

also added full api smoke test suite

ServerError is now the error base class. we've also kept TwirpError as
an alias.

also added full api smoke test suite
@davidzhao davidzhao requested a review from a team July 5, 2026 14:02

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Devin Review found 1 potential issue.

Open in Devin Review

Comment on lines 133 to 135
client_timeout = aiohttp.ClientTimeout(
total=timeout if timeout else DEFAULT_RINGING_TIMEOUT
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🚩 accept_whatsapp_call timeout lacks the RINGING_TIMEOUT_MARGIN used by SIP service

The accept_whatsapp_call method uses DEFAULT_RINGING_TIMEOUT (30s) as the request timeout when wait_until_answered is set, without adding RINGING_TIMEOUT_MARGIN (2s). In contrast, the SIP service's create_sip_participant uses ring + RINGING_TIMEOUT_MARGIN to ensure the HTTP request outlasts the ringing window. If the server takes exactly 30s to ring, the connector request could time out at the boundary. This is pre-existing behavior (unchanged by this PR) but the inconsistency is worth noting since the PR cleaned up the DialRequest union that previously included AcceptWhatsAppCallRequest.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant