Skip to content

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.

Sign up for GitHub

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

Open
LocutusOfBorg wants to merge 1 commit into
base: master
Choose a base branch
from
+2 −2
Open

test_dltctrlmsg: For some reasons using chars fails with int on some … #706

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions qdlt/tests/test_dltctrlmsg.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ TEST(CtrlPayload, parse) {

auto ctx = app.ctxs[0];
EXPECT_EQ(ctx.id, "MGR");
EXPECT_EQ(ctx.logLevel, '\xff');
EXPECT_EQ(ctx.traceStatus, '\xff');
EXPECT_EQ(ctx.logLevel, -1);
Copy link
Collaborator

@vifactor vifactor Jul 29, 2025

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.

Copy link
Contributor Author

@LocutusOfBorg LocutusOfBorg Jul 30, 2025

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

Copy link
Collaborator

@vifactor vifactor Jul 30, 2025

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?

Copy link
Contributor Author

@LocutusOfBorg LocutusOfBorg Jul 31, 2025

Choose a reason for hiding this comment

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

not at all :)

Copy link

@RubenGarcia RubenGarcia Oct 7, 2025

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..

Copy link

@RubenGarcia RubenGarcia Oct 7, 2025

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.

Copy link
Collaborator

@vifactor vifactor Oct 7, 2025

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?

Copy link

@RubenGarcia RubenGarcia Oct 10, 2025 •
edited
Loading

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.

EXPECT_EQ(ctx.traceStatus, -1);
EXPECT_EQ(ctx.description, "Context of main dlt system manager");
}
Loading