-
Notifications
You must be signed in to change notification settings - Fork 162
Fixes Exceptions and permanent Hang when parsing bad input, Adds bad input tests #197
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
This comment has been minimized.
This comment has been minimized.
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.
Looks mostly fine, just a few comments, left me know what you think.
This comment has been minimized.
This comment has been minimized.
Fixes #198 |
This comment has been minimized.
This comment has been minimized.
@JlKmn did you see some of the other comments? |
@milkshakeuk I think i answered to all of them, what am I missing? |
@JlKmn |
@milkshakeuk Very strange they didn't show for me on brave and edge. I can hover over the link and see some of the comment but can't click on it. They also don't show for me in Files changed with comments? |
Can you comment them 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.
@JlKmn
Looks like I needed to "finish the review" which is why you maybe couldn't see the comments.
This comment has been minimized.
This comment has been minimized.
@JlKmn |
…handled exception bugs and the permanent Hang
@milkshakeuk should be fine now right? |
Unit Test Results 5 files 56 suites 8s ⏱️ Results for commit fbde865. |
@JlKmn Yes thanks! |
@JlKmn I have rebased my branch from master so it now includes your changes however now some of my tests are failing because of changes I have made to |
@milkshakeuk ok how do we go about that |
@JlKmn Once my PR is ready ill push up a branch which you could fork and have a look at? |
@milkshakeuk ok |
@JlKmn ok here is my branch. |
@milkshakeuk ok i will look at it soon |
@milkshakeuk I looked at it and the changes in exceptions for the tests happen because you rewrote the Parse function in ParserBase.cs that gets called for every BadInput test. One fix would be to extend the one test to accept EncodingNotSupported because it is a HL7 child exception. Maybe move the others to the Gracefully test? |
@JlKmn ok I'll look to make those changes. |
@milkshakeuk Yes |
@JlKmn ok the PR has been merged. |
@milkshakeuk I will do a Fuzzing run on the new Parser tonight, we'll see tomorrow |
Fixes #196 and #198