Fix J1939Reader #140
Fix J1939Reader #140
Conversation
sophokles73
force-pushed
the
fix_J1939Reader
branch
from
57ae848
to
fd9a176
Compare
There was a problem hiding this comment.
Codewise it looks good to me, but couldn't test it @erikbosch can you do it?
dbc2val/test/test_dbc/test1_2.kcd
Outdated
| @@ -1,5 +1,24 @@ | |||
| <?xml version="1.0" encoding="UTF-8" standalone="yes"?> | |||
| <NetworkDefinition xmlns="http://kayak.2codeornot2code.org/1.0"> | |||
| <!-- | |||
| Copyright (c) 2023 Robert Bosch GmbH | |||
There was a problem hiding this comment.
@SebastianSchildt - for new files, do we want this form or the more generic "Copyright (c) 2023 Contributors to the Eclipse Foundation"
There was a problem hiding this comment.
good point 👍
the other Eclipse projects I am involved in usually take the more generic approach and let contributors add their affiliation to the NOTES.md file ...
There was a problem hiding this comment.
We have for some time now been using the generic "Copyright (c) 2023 Contributors to the Eclipse Foundation" for all new files, as that seems to be the current best practice.
It is however currently neither enforced nor written down as a decision somewhere, we just went where the river took us.
There was a problem hiding this comment.
Ok, I will adapt the PR accordingly.
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. |
|
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 ;-) |
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.
sophokles73
force-pushed
the
fix_J1939Reader
branch
from
fd9a176
to
a914170
Compare
|
@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 :-) |
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.