-
-
Notifications
You must be signed in to change notification settings - Fork 168
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
Adjust assertion of max_error_length test #480
Conversation
…fore the second :.
Good catch :) |
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.
@angatha could you provide an example file ics with a ': ' and add it to the parameter list? e.g:
BEGIN:VCALENDAR
BEGIN:VEVENT
SUMMARY:Example calendar with a ': ' in the summary
END:VEVENT
BEGIN:VEVENT
SUMMARY:Another event with a ': ' in the summary
END:VEVENT
[...]
END:VCALENDAR
I can do that. The one that you provided, is a good reference, but I'm missing the advantage of that. Assuming the state of #473, that test would succeed. And assuming the state before #473, where long messages where generated, that test will still pass, since the error is in the test itself. The fact, that the test succeeds, indicates the bug inside the test but no one will notice this since all tests passed. |
Oh, you're right. |
But it doesn't hurt :) |
Pull Request Test Coverage Report for Build 3354951542
💛 - Coveralls |
maybe we should assert that the message is split as we think in the test? or would that be testing tests esentialy? |
Tests are different from contracts or verifications or proofs of programs. They are a heuristic. If they fail, something is wrong if they don't, we do not know. I think, we are hitting that here. |
It looks good to me merging. |
Some message parts may be hidden from the assertion because of multiple splitting. See #473 and thisfor context.