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
Surface exceptions from Cython to Python as much as possible #16971
Conversation
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anything to be done in the way of test coverage?
@nathanielmanistaatgoogle You are right. I am going to add tests for exceptions that raised by ourselves. |
fa22abf
to
12bb39e
Compare
|
|
|
|
12bb39e
to
8ffebd3
Compare
|
|
8ffebd3
to
14369c4
Compare
|
14369c4
to
5ec4f40
Compare
|
|
|
Hi @nathanielmanistaatgoogle , the best way to test this PR is collecting the cases that people use our public API and their exception got ignored. Currently, I found three of them. The hard part is that if we know the exception will be ignored we already fixed it... So, do you have any insight about this? |
@lidizheng I feel that is a sensible thing to do. Testing cannot prove absense of all unknown bugs. |
@mehrdada can you elaborate your idea more? Shall we move forward with this PR or wait and collect more corner cases? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
@lidizheng The more test cases we have the merrier, but no, I think we should move forward sooner than later, as this is significant progress as is. All our existing tests will also be a safety net as they will see an exception if the Cython layer previously silenced it. |
5ec4f40
to
a5836ed
Compare
|
|
|
@lidizheng ping |
@mehrdada PTAL, The unit test has been updated. |
@nathanielmanistaatgoogle When you got a chance, can you take a look at this PR again? |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mostly LGTM, but perhaps the unit tests are better off folded into the relevant existing test file each instead (metadata test and channel test) instead of a separate file (which would not make sense if it weren't an artifact of project's evolution).
385a7c8
to
3a5a5a4
Compare
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
3a5a5a4
to
3966d59
Compare
3966d59
to
3adca9f
Compare
|
|
Test has been added
except *
is not needednogil
andexcept *
is under investigationIssues #16643