-
Notifications
You must be signed in to change notification settings - Fork 265
test_dltctrlmsg: For some reasons using chars fails with int on some … #706
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
base: master
Are you sure you want to change the base?
Conversation
| EXPECT_EQ(ctx.id, "MGR"); | ||
| EXPECT_EQ(ctx.logLevel, '\xff'); | ||
| EXPECT_EQ(ctx.traceStatus, '\xff'); | ||
| EXPECT_EQ(ctx.logLevel, -1); |
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.
I think, the problem is not here. I guess the byte layout generated by dlt-daemon on a big endian platform for the same message will be different, i.e. data-content for the same logLevel and traceStatus will be somewhat altered there.
Question: are you developing dlt-viewer on big-endian platform? If you are just using it there (and not developing), I do not think the tests matter.
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.
I am building dlt-viewer on Debian, so for both LE and BE architectures, and I would like to execute tests for all archs, to avoid missing regressions...
This is the arch lists:
https://buildd.debian.org/status/package.php?p=dlt-viewer&suite=experimental
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.
tbh, I do not completely understand why it fails on BE. \ff is decimal -1 for int8_t type. Do you understand the fix?
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.
not at all :)
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.
On some platforms, char is signed and in some it is unsigned. You may have a weird cast -1 => 255 and then another cast to int, where the final comparision -1 == 255 fails..
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.
On other platforms, chars are 16 bit, so comparing 0xff to -1 will also fail.
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.
@RubenGarcia thanks for the info. Do you have an idea how the test can be rewritten so that it remains valid both on BE and LE platforms?
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.
The important thing is that you write down the types of the data that you receive, and understand the conceptual data types.
If the documentation says the thing is supposed to be an unsigned byte, then cast it to uint8_t and only compare it to 255.
I do not like EXPECT_EQ(ctx.logLevel, '\xff'); because that uses either char or wchar_t which you have no control over.
I also do not like EXPECT_EQ(ctx.logLevel, -1); because you don't have control over signedness.
Go to the documentation, verify what exact type ctx.logLevel is, and then use
EXPECT_EQ((type)ctx.logLevel, (type)(<value that the documentation says should be there>));
It would be best if type is platform agnostic, but since you cast both sides, it should work even if the type changes per-platform.
Don't remove EXPECT_EQ(ctx.traceStatus, '\xff');, but change it to what the documentation says it should have.
…architectures (such as arm* ppc64el, s390x) Closes: COVESA#705
…architectures (such as arm* ppc64el, s390x)
Closes: #705