Skip to content
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

Test to distinguish equality validation of bytes vs. strings. #93

Merged
merged 2 commits into from
Apr 20, 2021

Conversation

ams-tschoening
Copy link
Contributor

This makes sure that a textual error message is thrown instead of HEX-bytes. valid_fail_eq_str_spec.rb is created manually to be able to check for concrete error messages, no idea if this is possible using KST already.

kaitai-io/kaitai_struct_ruby_runtime#7

…kes sure that a textual error message is thrown instead of HEX-bytes.

kaitai-io/kaitai_struct_ruby_runtime#7
}.to raise_error(
# Make extra sure that handling of "byte[]" is not used by looking at the message.
Kaitai::Struct::ValidationNotEqualError,
a_string_including('"BACK"', '"PACK"')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would definitely compare the full message. This mere testing whether it contains specific strings constitutes too much creativity, and it's not very well convertible to the KST language-agnostic approach (when we decide to implement it in the KST translator).

BTW, for KSC errors we have ErrorMessagesSpec, which tries to compile malformed KSY specs in the kaitai_struct_tests / formats_err/ directory and compares them to the expected error message (for example, kaitai_struct_tests / formats_err/attr_bad_id.ksy). It also asserts the full message.

Anyway, it's definitely a good idea to extend the tests in the current KS test suite to also ensure that the error messages are consistent between all target languages (because as it will surely turn out, they're not).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would definitely compare the full message.[...]

I've decided against this explicitly because error messages are pretty inconsistent currently and might get normalized at some point. Most people don't care too much about things like lower vs. upper case of phrases, spelling, if it's complete sentences or not etc. What we DO care heavily instead is IF textual strings are printed human readable or using HEX-bytes. And the two phrases currently checked for focus exactly on that.

Why should the current test fail if wrong textual content is still printed human readable, but with some slightly different error message? Makes it unnecessary difficult to maintain the test in my opinion. Especially because the current approach of using a_string_including is a PoC only anyway and not supported by KST. Before checking concrete error messages across languages, one needs to decide how to make that concept available in KST in the long term.

Up until that point I suggest keeping things as easy as they are currently and focus on the important HEX vs. not-thing.

Copy link
Member

@generalmimon generalmimon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's go with this. Thanks for the contribution!

@generalmimon generalmimon merged commit 5c1ae09 into kaitai-io:master Apr 20, 2021
@ams-tschoening ams-tschoening deleted the valid_fail_eq_str branch April 21, 2021 08:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants