search

eclipse / kuksa.val.feeders

kuksa.val.feeders
Apache License 2.0
8 stars 22 forks source link

Fix J1939Reader #140

Closed sophokles73 closed 9 months ago

sophokles73 commented 10 months ago

The reader erroneously referred to a non-existing (private) member of the DBCReader. This has been fixed.

Also added some tests for verifying the J1939Reader's functionality.

erikbosch commented 9 months ago

Codewise it looks good to me, but couldn't test it @erikbosch can you do it?

That is part of our current problem for J1939 - we do not have any "real" (or a faked) public log file or signal definitions, so we need to rely on unit tests and on that the contributor has performed more exhaustive end-to-end testing. So we have problems doing end-to-end tests.

sophokles73 commented 9 months ago

Well, I tried to start alleviating the situation by adding some unit tests for the j1939 processing. For verifying the correctness of the code, IMHO it should make no difference if we use real SAE J1939 message/signal definitions or synthetic ones.

I agree, however, that the test suite could be a little more thorough ;-)

sophokles73 commented 9 months ago

@erikbosch anything else I need to change?

erikbosch commented 9 months ago

@erikbosch anything else I need to change?

No, looks good to me. I like that you add unit tests.

Concerning testing, it is true that it does not matter if it is real data or faked data, but it would be good to have a j1939 log file + message definition so that we could do a real end-to-end test as part of release testing sanity checks, similar to how we do for the regular included log file for dbcfeeder. Like start dbcfeeder in j1939 mode with a j1939 log file as input and verify that expected values actually are sent to Databroker. But that is more of a long term goal :-)