-
Notifications
You must be signed in to change notification settings - Fork 3k
Property-based tests for the base64 module #5967
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
Conversation
CT Test Results 2 files 85 suites 29m 58s ⏱️ Results for commit c43bac2. ♻️ This comment has been updated with latest results. To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass. See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally. Artifacts// Erlang/OTP Github Action Bot |
bjorng
left a comment
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.
Overall looks good to me.
The base64 module has been optimised for speed (but it is still considered too slow) and no attempt has been made to produce nice error messages. With improvements in the compiler in recent releases, it might now be possible to produce more consistent error behaviour without compromising performance, but that should be a the subject of a separate pull request.
|
@bjorng ok, I think I did all the requested changes 😃 |
|
Thanks! Can you squash the commits? We will test this PR in our daily builds after the release of Erlang/OTP 25. |
52a546f to
c43bac2
Compare
Done 👍
Ok, looking forward to it (OTP/25, too 😉) |
|
Thanks for your pull request. |
As the title says, this PR introduces property-based tests for the
base64module.Test coverage is around ~95%, looks like some obscure edge cases that may occur in relation to
mime_decodeare rarely created by the generators. However, those are covered by the normal CT suite.Some things I noticed while working on this:
badargwhich is fine and expected, but there is alsobadarith,function_clause,case_clauseetc bubbling up from internal functions. This multitude of error reasons makes handling them tedious (and so, lazy as I am, I settled on catchingerror:_after hitting three different error reasons). What (I guess) I'm trying to say is, they essentially all boil down to the argument being somehow bad, and so it may be good to unite them all under thebadargreason.mime_decode/1andmime_decode_to_string/1, the relaxed siblings of the pedanticdecode/1anddecode_to_string/1functions, are surprisingly strict when it comes to padding, or rather missing padding. In reality (that is informally, which in turn is IMO), padding is not strictly required if you know that your input is Base64. So it may be opportune to lift that restriction for the relaxed decoding functions.