-
Notifications
You must be signed in to change notification settings - Fork 270
test_dltctrlmsg: For some reasons using chars fails with int on some ...#706
test_dltctrlmsg: For some reasons using chars fails with int on some ...#706LocutusOfBorg wants to merge 1 commit intoCOVESA:masterfrom
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)(
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.
|
Can this be merged into main branch? |
|
Let's ensure tests succeed on all platforms before merging. |