-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Fix incorrect check of InvalidFieldException to InvalidFieldFault while generating MSQ Error Report #16273
Fix incorrect check of InvalidFieldException to InvalidFieldFault while generating MSQ Error Report #16273
Conversation
Can we add any unit tests here? It feels weird that we have to test it in a separate custom build to generate the exception. Can you mention where you are throwing an exception to test this flow? It'd be prudent to add a unit test that throws in the same flow to test this exception. |
Have added |
@VisibleForTesting | ||
boolean writeData() |
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.
I think we should revert this change, and look for a way to generate the exception using the class's public methods (#11848 (comment)), if possible. Since the code coverage is still not satisfied, I think we can remove the new test added (since that isn't testing the actual code in the ControllerImpl where everything is bubbled up to) and merge it as is, since it's a fairly low risk change
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 for the comment and the reference. Using a public method now instead.
Failures are unrelated. Thanks for the PR 🚀 |
InvalidFieldFault
is incorrectly checked asInvalidFieldException
inmapQueryColumnNameToOutputColumnName
. Fix it to the correct check. Also added test for the generated exception as part of RowBasedFrameWriterTest.Verified the generated MSQErrorReport by raising an empty RuntimeException inside
StringFieldWriter.writeTo()
in a custom build.