Dark Mode

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Open
LocutusOfBorg wants to merge 1 commit intoCOVESA:masterfrom
LocutusOfBorg:master
Open

test_dltctrlmsg: For some reasons using chars fails with int on some ...#706
LocutusOfBorg wants to merge 1 commit intoCOVESA:masterfrom
LocutusOfBorg:master

Conversation

Copy link
Contributor

LocutusOfBorg commented Jul 28, 2025

...architectures (such as arm* ppc64el, s390x)

Closes: #705

LocutusOfBorg mentioned this pull request Jul 28, 2025
alexmucde added this to COVESA DLT Viewer Jul 29, 2025
github-project-automation bot moved this to ToDo in COVESA DLT Viewer Jul 29, 2025
alexmucde added the bug label Jul 29, 2025
alexmucde added this to the Release v2.29.0 milestone Jul 29, 2025
vifactor reviewed Jul 29, 2025
EXPECT_EQ(ctx.id, "MGR");
EXPECT_EQ(ctx.logLevel, '\xff');
EXPECT_EQ(ctx.traceStatus, '\xff');
EXPECT_EQ(ctx.logLevel, -1);
Copy link
Collaborator

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 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 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 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 :)

vifactor reacted with laugh emoji
Copy link

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 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 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 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)());
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.

vifactor added the help wanted label Jul 31, 2025
...architectures (such as arm* ppc64el, s390x)

Closes: COVESA#705
LocutusOfBorg force-pushed the master branch from 80df6e1 to 68bb92d Compare August 4, 2025 08:30
vifactor removed this from the Release v2.29.0 milestone Nov 10, 2025
Copy link
Collaborator

bunty95 commented Feb 5, 2026

Can this be merged into main branch?

Copy link

RubenGarcia commented Feb 6, 2026

Let's ensure tests succeed on all platforms before merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

vifactor vifactor left review comments

+1 more reviewer

RubenGarcia RubenGarcia left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

Status: ToDo

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

New test fails on non amd64 nor i386 platforms

5 participants