-
Notifications
You must be signed in to change notification settings - Fork 618
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
Improve readability of proto decode exception message #2768
Improve readability of proto decode exception message #2768
Conversation
5ce64b3
to
453e164
Compare
@sandwwraith can you take a look on this PR? |
@xiaozhikang0916 sure |
It seems there are some tests failing |
formats/protobuf/commonMain/src/kotlinx/serialization/protobuf/internal/Helpers.kt
Show resolved
Hide resolved
formats/protobuf/commonMain/src/kotlinx/serialization/protobuf/internal/Helpers.kt
Outdated
Show resolved
Hide resolved
formats/protobuf/commonMain/src/kotlinx/serialization/protobuf/internal/Helpers.kt
Show resolved
Hide resolved
formats/protobuf/commonMain/src/kotlinx/serialization/protobuf/internal/ProtobufDecoding.kt
Outdated
Show resolved
Hide resolved
formats/protobuf/commonMain/src/kotlinx/serialization/protobuf/internal/ProtobufDecoding.kt
Outdated
Show resolved
Hide resolved
formats/protobuf/commonMain/src/kotlinx/serialization/protobuf/internal/ProtobufReader.kt
Outdated
Show resolved
Hide resolved
formats/protobuf/commonTest/src/kotlinx/serialization/protobuf/ProtoTagExceptionTest.kt
Outdated
Show resolved
Hide resolved
formats/protobuf/commonTest/src/kotlinx/serialization/protobuf/ProtoTagExceptionTest.kt
Outdated
Show resolved
Hide resolved
formats/protobuf/commonTest/src/kotlinx/serialization/protobuf/TestFunctions.kt
Outdated
Show resolved
Hide resolved
formats/protobuf/commonTest/src/kotlinx/serialization/protobuf/TestFunctions.kt
Outdated
Show resolved
Hide resolved
val currentMessage = message[index] | ||
assertNotNull(currentException, "Expected exception to have a cause with message $currentMessage, but it was null") | ||
require(currentException != null) | ||
assertEquals(currentMessage, currentException.message, "Expected exception to have a cause with message <$currentMessage>, but it was <${currentException.message}> at cause stack ${index + depth}") |
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.
IDEA can show nice 'Click to see the difference' pop-up to compare between expected and actual values on test failure, but only if the exception message satisfies JUnit4's exception message regex (https://stackoverflow.com/questions/10934743/formatting-output-so-that-intellij-idea-shows-diffs-for-two-texts). So you may want to rephrase your message a bit for it to include expected:<bla-blah> but was:<blah-blah-blah>
substring.
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.
After diving into assertEquals
I found that it would append the Expected <$expected>, actual <$actual>.
string after message passing in. So I would just simplify the input message.
It works in my IntelliJ IDEA 2024.2.1 RC and you can comment out the Ignore
annotation in the new TestFunctionTest
to see if it also works in other environments. And since these tests are for checking IDEA behaviours, I would just leave it failing, not wraping another assertFailWith
with message check.
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 all your efforts!
Found it hard to debug when exceptions with message like
Expected wire type 0, but found 1
were got. This PR is about to make some improvement of these messages for better tracability and readability.ProtoWireType
to show both name and id in message,ProtobufDecodingException
in mostdecodeXXX
functions, with proto number and type name in new exception message.