Test data contains invalid messages, but should not

[marcel-kanter]

Have a look at

logformats_test

def test_can_error_frame_ext(self):
    expected = can.Message(
        timestamp=2459565876.494607,
        is_error_frame=True,
        arbitration_id=0x19999999,
        is_extended_id=True,
        channel=0x1110,
        **dlc=0x66**,
        data=[0xCC, 0xDD, 0xEE, 0xFF, 0x11, 0x22, 0x33, 0x44],
    )
    actual = self._read_log_file("test_CanErrorFrameExt.blf")
    self.assertMessagesEqual(actual, [expected] * 2)
    self.assertEqual(actual[0].channel, expected.channel)

Even with an FD frame and treading dlc as length, this an invalid message. But code seems not to expect an invalid message.

[marcel-kanter]

Another one. The dlc values were missing.

test/data/example_data.py

TEST_MESSAGES_REMOTE_FRAMES = sort_messages(
[
Message(
arbitration_id=0xDADADA,
is_extended_id=True,
is_remote_frame=True,
dlc=1,
timestamp=TEST_TIME + 0.165,
),
Message(
arbitration_id=0x123,
is_extended_id=False,
is_remote_frame=True,
dlc=1,
timestamp=TEST_TIME + 0.365,
),
Message(
arbitration_id=0x768,
is_extended_id=False,
is_remote_frame=True,
dlc=1,
timestamp=TEST_TIME + 3.165,
),
Message(
arbitration_id=0xABCDEF,
is_extended_id=True,
is_remote_frame=True,
dlc=1,
timestamp=TEST_TIME + 7858.67,
),
]
)

[marcel-kanter]

Am I missing something? Is this intended?

[christiansandberg]

The BLF files are borrowed from the C++ library’s unit tests since I don’t have a Vector tool to generate new ones with. The DLC change makes sense. Could you provide a pull request?

[marcel-kanter]

I'm working on a fix already. It seems there are more bugs in the reader/writers, they hide behind the defacto non-existent check of the properties of Message, a behavior I will remove, since it makes no sense to create messages that are corrupt and (physically on the can bus) not possible.

[marcel-kanter]

Leaving is as it leads in the end to a situation in which no-one can rely on the properties of a Message instance. This means you have to implement the checks in the interfaces. That will definitvely produce a lot of duplicate error-prone code.

[christiansandberg]

I believe the checks were disabled by default due to performance reasons. The interfaces and log formats should be implemented so that they do not produce incorrect data. If there for some reason are incorrect data then the application should continue to function and not raise an exception.

We could make the check method public so that the application can check any incoming message if desired.

[marcel-kanter]

Hmm. Well not so good idea, for reasons i stated before.
One other thought: Is it possible, that the data files for the blf are negative examples?
Found another incorrect Message. is_fd and is_remote_frame are not allowed together. Remote frames have been dropped in can FD due to the arbitration problems when simultaneous RTR are transmitted.

[marcel-kanter]

Ok. ASC Reader/Writer work now. I will produce some test data sets and add them to the test code.

[christiansandberg]

Bugs in the log formats code should be caught by unit tests, not during runtime. That’s why I think we can call the check method explicitly in the unit tests to make sure they contain valid data. Forcing runtime checks and the overhead it implies when the data usually can be assumed to be valid is probably not what people want.

[christiansandberg]

The BLF files are not negative tests. The C++ library doesn’t assume anything about what a valid message is so the data is just some random values. If you could generate new files with valid data that would be very much appreciated.

[marcel-kanter]

The file test_CAnMesage.blf is kind of corrupt. It matches the format description vector provides, but their tools wont import them. Basically because the DLC of 51 is not valid. At least manipulating the file with an hex editor to valid values let canalyzer import them.
I'll need the full licenced version from work to generate more valid sample files. The demo version encrypts the blf files.

[marcel-kanter]

I believe the checks were disabled by default due to performance reasons. The interfaces and log formats should be implemented so that they do not produce incorrect data. If there for some reason are incorrect data then the application should continue to function and not raise an exception.

We could make the check method public so that the application can check any incoming message if desired.

The point is that there are more than 6485 lines of code in this package and check is only used in the test of the Message class itself (which is very incomplete btw.). No-one used this yet elsewhere, so making the method optional will not propably improve the quality. And the performance penalty will be there anyway, since no interface and no io can trust the objects it gets and must check them anyway.

[marcel-kanter]

So before I continue to fix this, is there any chance that the always enabled check is going into this repo?

[christiansandberg]

I would like someone else, maybe @hardbyte or @felixdivo, to chime in on that. In my opinion I think checking of our internal sources should be optional, i.e. there should be a check() method the application can call to verify it is correct. Otherwise we just pass the messages as is however the source generated it.

[felixdivo]

Hi folks,

the last two major discussions on the message classes happened in #413 (adds Message.check()) and #497 (cleanups to that method). We did not directly discuss if check=True should be the default in the constructor, but I think we did that somewhere.

I'm favoring the automatic checking by default in the constructor (make check=True the default), as it allows bus implementations and users to trust that messages are well-formed. If anyone knows what he's doing and wants to tweak performance, he could pass check=False to the constructor do disable the check. We could optionally do that in buses, but should probably not trust users with creating valid messages.

If you could generate new files with valid data that would be very much appreciated.

Yes! Generally, I think that the internal tests should contain valid messages, as some might use it for reference and should find proper code.

[I was in vacation for a few days, thus the late answer.]