Skip to content

[SYNPY-1581] remove try: except: blocks that log and raise exceptions #1203

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

Merged
merged 1 commit into from
May 15, 2025

Conversation

SageGJ
Copy link
Contributor

@SageGJ SageGJ commented May 14, 2025

Problem:

When client functionality was called by a user and wrapped in a try: catch: block, the exception was caught but the exception trace was still displayed. This behavior was undesirable by the user who expected no trace to be displayed since she had implemented her own error handling.

Solution:

The functionality in the wrapper that logs and raises exceptions was removed to allow exceptions raised in wrapped async methods to be handled by the caller or within the client by the interpreter instead of having an intermediary that always logs and raises exceptions, which is undesirable by some users
Design Doc

Testing:

testing was done with a mre adapted from the linked ticket. When executed no trace is displayed and the exception is caught

@BryanFauble
Copy link
Member

@SageGJ When you get a chance do you mind rephrasing the PROBLEM portion of the PR description. This is more so the solution was implemented. Problem should ideally focus on the reasons why this change took place

@SageGJ SageGJ marked this pull request as ready for review May 15, 2025 18:20
@SageGJ SageGJ requested a review from a team as a code owner May 15, 2025 18:20
@SageGJ
Copy link
Contributor Author

SageGJ commented May 15, 2025

On the initial run, there was a test failure that was not present on subsequent runs. On the next run tests were canceled despite none of the other tests failing. has this behavior been observed before?

Copy link
Member

@BryanFauble BryanFauble left a comment

Choose a reason for hiding this comment

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

LGTM!

@thomasyu888
Copy link
Member

thomasyu888 commented May 15, 2025

On the initial run, there was a test failure that was not present on subsequent runs. On the next run tests were canceled despite none of the other tests failing. has this behavior been observed before?

@SageGJ I did not look deeply into the failed test but I'll just add that through observation there are flakey tests due to parallel execution of tests that interact with Synapse resources. Due to this, it sometimes causes conflict and the tests fail. It passes when you turn it off and on again.

I also think CI that is running are automatically canceled when you push a new commit? (I could be wrong)

@BryanFauble
Copy link
Member

On the initial run, there was a test failure that was not present on subsequent runs. On the next run tests were canceled despite none of the other tests failing. has this behavior been observed before?

@SageGJ I did not look deeply into the failed test but I'll just add that through observation there are flakey tests due to parallel execution of tests that interact with Synapse resources. Due to this, it sometimes causes conflict and the tests fail. It passes when you turn it off and on again.

Yup!

I also think CI that is running are automatically canceled when you push a new commit? (I could be wrong)

Thats right

@SageGJ
Copy link
Contributor Author

SageGJ commented May 15, 2025

I did not look deeply into the failed test but I'll just add that through observation there are flakey tests due to parallel execution of tests that interact with Synapse resources. Due to this, it sometimes causes conflict and the tests fail. It passes when you turn it off and on again.

Perfect, thanks for that context @thomasyu888 !

I also think CI that is running are automatically canceled when you push a new commit? (I could be wrong)

There's only one commit in this pull request, the subsequent tests were triggered manually through the actions tap in the repo

cc @BryanFauble

@SageGJ SageGJ merged commit 6a4e858 into develop May 15, 2025
28 checks passed
@SageGJ SageGJ deleted the SYNPY-1581-async-exception-handling branch May 15, 2025 20:31
@thomasyu888 thomasyu888 changed the title remove try: except: blocks that log and raise exceptions [SYNPY-1581] remove try: except: blocks that log and raise exceptions May 16, 2025
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.

3 participants