fixed #13376 - include --check-level in analyzer information hash#7072
fixed #13376 - include --check-level in analyzer information hash#7072firewave merged 1 commit intocppcheck-opensource:mainfrom
--check-level in analyzer information hash#7072Conversation
| toolinfo << (mSettings.severity.isEnabled(Severity::information) ? 'i' : ' '); | ||
| toolinfo << mSettings.userDefines; | ||
| toolinfo << std::to_string(static_cast<std::uint8_t>(mSettings.checkLevel)); | ||
| // TODO: do we need to add more options? |
There was a problem hiding this comment.
I filed https://trac.cppcheck.net/ticket/13379 about this.
There was a problem hiding this comment.
how about creating a Settings::toolinfo() method and unit testing that?
There was a problem hiding this comment.
That would be a rather pointless test. It returns exactly what we expect it to return based on the input so it would just be a logic duplication.
There's more refactorings coming so maybe one of those will give us an in for a useful test.
There was a problem hiding this comment.
your test only checks that --check-level=normal / --check-level=exhaustive works as expected. With such unit test we can more quickly test that all the flags we want to test influence the output. I would not think that pytest tests that cover each flag would be nice. 1 pytest that checks that the everything works as expected if toolinfo is different is enough imho.
There was a problem hiding this comment.
But the test is rather pointless as you need to replicate the whole logic of the function. And it would not be the final hash. Also that test would break soon as the suppressions need to be split from the settings and would just complicate things.
And we should actually have not a single test in Python but one for each possible values as each of them might have side effects which would not be visible if you try to test it as whole.
This is only addressing part of this because a user encountered this. This needs a lot more work and a first step which tests this in a more generic will follow soon.
There was a problem hiding this comment.
If you are really against the Settings::toolinfo().. if you expose a calculateHash in analyzerinfo and unit test that then I am also happy. I want to have unit tests that is my motivation.
There was a problem hiding this comment.
That requires some refactoring first.
I would like to merge this fix before that as it was reported by a user and not discovered internally.
There was a problem hiding this comment.
okay.. feel free to merge and then refactor.
There was a problem hiding this comment.
We can still remove/merge it later on - more things need to be tested in future anyways. Too much tests is usually not the issue we are having. 🤪
There was a problem hiding this comment.
Too much tests is usually not the issue we are having.
Yes I do agree however I want to also say I would not like to just throw in lots of random tests in random places. Let's continue to structure it as much as we can and make the tests as clean and simple as possible.
As far as I see there is no existing pytest test for calculateHash yes that is really unfortunate.
2 participants
No description provided.