-
Notifications
You must be signed in to change notification settings - Fork 143
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
Make tests stop build on error #165
Make tests stop build on error #165
Conversation
I like this change, but i want to add normalized output into test before merging this. |
I'm not sure what normalized outputs means. If you modify the output, you will have to touch all "golden files". |
mbus_parse_hex supports a option -n. This supports parsing the M-Bus frame the new (normalized value) way found in mbus-protocol-aux. The normalized output is better for machine processing, while the default output (mbus-protocol) is more intended for humans.
I understand but your check would only handle half of the truth.
No, we need to add all the normalized output files. |
Here as an example for allmess_cf50.hex default output:
normalized output:
|
The necessary changes for generate-xml.sh are available in #168 |
bc1dc0a
to
d114d53
Compare
Usefull when placing the output in another directory.
Updated/rebased onto rscada/generate-norm-xml. Found one or two things that have bothered me. Please review! |
da6b744
to
6c1e202
Compare
Usefull when placing the output in another directory.
For enhanced readability, split the commands over several lines.
Pushed a new version |
@lategoodbye : Can you review this PR please? |
test/generate-xml.sh
Outdated
fi | ||
|
||
# Check that there was exactly the correct number of files that failed to parse | ||
EXPECTED_PARSE_ERRORS=4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fact that we expect parse errors is bad. I suggest to move the relevant hex files into unsupported-frames.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There can be a point in doing negative tests.
I see that we have two more directories, unsupported-frames and error-frames.
Should we do execute the code for these directories as well?
Since the files are named: invalid_length.hex
and invalid_length2.hex
, maybe error-frames/
is more appropriate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, negative tests are a good point. The only one we currently have are invalid_length*.hex
If you want to integrate this in the pull request make two different directories like test-frames-good and test-frames-bad. But if you go this way, this logic should good out of generate-xml.sh
error-frames are valid errors messages send by the slave. The mentioned hex files doesn't fit in there. unsupported-frames are for currently unsupported frames so executing code on these directories doesn't make sense to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a simple if statement for the names of the directories that should contain errors.
It seem simplest to keep this information in this script, otherwise we would have some other script know that error-frames should have 30 parse errors, and pass that information to generate-xml.sh.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fact that we code the expectation of x parse errors is the whole problem. I don't want this in a script to generate XML files. Please keep it simple and easy to maintain. Also i don't want that we run generate-xml on the unsupported frames as a test. Except of that i'm happy to pull.
Usefull when placing the output in another directory.
For enhanced readability, split the commands over several lines.
e0e8a50
to
933da03
Compare
Usefull when placing the output in another directory.
933da03
to
a2cd149
Compare
a2cd149
to
50e3141
Compare
@lategoodbye , could you have another look? |
Usefull when placing the output in another directory.
For enhanced readability, split the commands over several lines.
50e3141
to
6c76468
Compare
@lategoodbye : New version, where I no longer check the count of errors, and instead accept that the two other directories does not exit with code zero. |
Usefull when placing the output in another directory.
For enhanced readability, split the commands over several lines.
Thanks |
Usefull when placing the output in another directory.
(cherry picked from commit 2d09cfc)
(cherry picked from commit 6901931)
Usefull when placing the output in another directory. (cherry picked from commit e89f929)
For enhanced readability, split the commands over several lines. (cherry picked from commit 3897ac7)
(cherry picked from commit f498cf1)
Usefull when placing the output in another directory.
For enhanced readability, split the commands over several lines.
Update script to make the automatic tests catch when testfiles should be updated.
(It just happend again.)