Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 2 commits into from

Conversation

fredrik-sk
Copy link
Contributor

@fredrik-sk fredrik-sk commented Jun 9, 2020

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.

It seems it was forgotten, and the automatic tests did not fail on it.

2f9fa5c Implement negative BCD number (Type A)
@fredrik-sk fredrik-sk changed the title Update golden test files after commit 2f9fa5c Update golden test files after commits 2f9fa5c, 23d8ee4 Jun 9, 2020
@fredrik-sk
Copy link
Contributor Author

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

@gumulka
Copy link
Contributor

gumulka commented Jun 10, 2020

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

@fredrik-sk
Copy link
Contributor Author

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
Copy link
Collaborator

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
Copy link
Contributor Author

@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 04eeda3 before 23d8ee4 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
Copy link
Contributor Author

@lategoodbye: In my next pull request: #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
Copy link
Contributor Author

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
Copy link
Collaborator

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

For me 2f9fa5c 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
Copy link
Contributor Author

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

@fredrik-sk
Copy link
Contributor Author

This seems to have been fixed by #167

@fredrik-sk fredrik-sk closed this Jun 28, 2020
@fredrik-sk fredrik-sk deleted the update-testfiles branch July 7, 2020 11:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants