search

rscada / libmbus

Meter-bus library and utility programs
http://www.rscada.se/libmbus
BSD 3-Clause "New" or "Revised" License
217 stars 137 forks source link

Update golden test files after commits 2f9fa5c, 23d8ee4 #164

Closed fredrik-sk closed 4 years ago

fredrik-sk commented 4 years ago

It seems it was forgotten to update the test file used by the, and the automatic tests did not fail on it.

2f9fa5c Implement negative BCD number (Type A) 23d8ee4 Added more medium definitions according to DIN EN 13757-7:2018-06 (#162) [UPDATED]

Another problem is that the automatic tests did not give an error on this, I have updates for that as well.

fredrik-sk commented 4 years ago

@gumulka : Can you confirm that commit 39372f379d745a5bb45953570f90f53ac88d2d35 correctly updates the golden files according to the change in commit 23d8ee4 ?

gumulka commented 4 years ago

It does look correct, but I did not have a deeper look into it.

fredrik-sk commented 4 years ago

In this pull request, I have updated the golden files so they match the current state of master, which has already been reviewed.

What is the process in order to get this merged? Who are allowed to accept it in code review? Do I have to find someone that can review this?

lategoodbye commented 4 years ago

In this pull request, I have updated the golden files so they match the current state of master, which has already been reviewed.

What is the process in order to get this merged? Who are allowed to accept it in code review? Do I have to find someone that can review this?

I will take care of the review. The whole problem is that we don't have "golden" test files, otherwise they don't need to be updated. I prefer to make sure that BCD decoding is correct, before updating the test files / pull this request.

fredrik-sk commented 4 years ago

@lategoodbye : What I have done is executed the code on the current master, and update the files in the repo so they don't give any difference. (The reason that I have two commits is that I created my commit 04eeda325f442559b8f2930d4a3c084763316763 before 23d8ee4844bfbaf0fef99993e8bb50ad19d2430c was merged). I don't touch any code, the decoing and printing has passed som kind of code review earlier.

I used the term golden files to refer to generated the .xml files that we do check in to the repository.

In the case of BCD encoding/decoding, it would be better to have normal unit tests, but that is some more work.

fredrik-sk commented 4 years ago

@lategoodbye: In my next pull request: https://github.com/rscada/libmbus/pull/165, I would like to make these tests to fail the build.

Making sure that one cannot updated the code without updating the generated .xml-files at the same time.

fredrik-sk commented 4 years ago

Any new regarding any of my three pull requests (164, 165, 166)? Can we do some kind of 'quid pro quo' to make something happen?

lategoodbye commented 4 years ago

@fredrik-sk Sorry, but during summertime the libmbus doesn't have high prio for me.

For me 2f9fa5ccc8aa032556ab77b7cafcb24670b6bf8d is broken. So we better find a compromise which means the old behavior plus signed BCD handling. After that the diff should be much understable.

fredrik-sk commented 4 years ago

So do you want to revert 2f9fa5c , cherry pick 39372f379d745a5bb45953570f90f53ac88d2d35, and make sure that all "golden files" are up to date?

fredrik-sk commented 4 years ago

This seems to have been fixed by https://github.com/rscada/libmbus/pull/167