refs #12358 - CI-unixish.yml: added step which checks for AST and ValueFlow changes

[firewave]

No description provided.

[firewave]

Since simplecpp.cpp is standalone and not too small it makes for a good file to run on. It does not contain much modern code at the moment but I still think this makes for a nice smoketest for the AST and ValueFlow internals we usually only verify by the way the findings change.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[danmar]

I spontanously don't have a good feeling about this. such golden files are not easy to maintain. so if we fix various tokenizer, ast and valueflow problems we will potentially have to update the golden file even though nothing is wrong. We will not spot problems that exist in such big data.

Write tests that check if we do the right thing and don't need constant maintenance..

[firewave]

I understand that but we need something like this. The test coverage is not enough and this makes e.f. ValueFlow optimization work safer. I had several cases where the output didn't change but it is was actually totally wrong. We need to catch such things before going in daca. And it might not even manifest in there.

I chose simplecpp since it is just a single file. So there is no need to manually edit files but just pull the newer version from the CI and overwrite it.

I wanted to maintain this externally but that will most certainly bitrot if it is not tied to the build.

[firewave]

See #7768 (comment) for a case where this found a subtle bug I would have introduced.

[danmar]

See #7768 (comment) for a case where this found a subtle bug I would have introduced.

I fear that there is a lot of flux in the cppcheck repo and while the intention is good it inevitably introduces bugs.

In that PR the bug was found because you looked carefully at the diff. If we just copy the golden file without looking at it then it does not help but is only in the way.

Can you implement this test somehow else. Maybe generate selfcheck.exp automatically based on cppcheck git head or whatever the PR target branch is. how about just showing the diff in a PR comment so it can be resolved/dismissed..

[danmar]

I wanted to maintain this externally but that will most certainly bitrot if it is not tied to the build.

that sounds somewhat interesting imho. I am not sure if bitrot has to be a problem. we could write a script that will compile and run cppcheck on every commit in the git log for last X days and then shows some report about which commits change the output..

This report could be generated in a nightly build.

but would anybody look at the report then?

[firewave]

Spoiler alert: I start to ramble in the later part of this comment because those realization just started coming in and I didn't feel like re-doing the whole post. Sorry about that.

Can you implement this test somehow else. Maybe generate selfcheck.exp automatically based on cppcheck git head or whatever the PR target branch is. how about just showing the diff in a PR comment so it can be resolved/dismissed..

The problem is that you need some kind of baseline. That means you need to build and run the previous version to generate that. We could store the binary and/or the output and pull that but I do want that dependency from a workflow into a previously build job (it might also not be reliable - we had issues with that within the same workflow in the same build). It would also increase the runtime and complexity of workflow if we have to pull and build the current version is it based off (and that also needs to consider branches and fork - maybe even more).

I thought about doing this as scheduled job (like IWYU or CSA) but that was with a much bigger scope (simplecpp is a more nicer contained example to use). That still would require the generation of the baseline and it would only be triggered periodically. And scheduling it off on each PR felt too disjoint and might only accumulate reports nobody even wants to check (see Coverity). The scheduled CSA is already piling up findings (it is only scheduled because it is too slow). IWYU is separate because it still has too many false positives (in simplecpp I was able to enable misc-include-cleaner from clang-tidy as it has a much smaller and manageable scope). And if those changes were made by one-off contributors those things might never be fixed because don't even know what the intended behavior is supposed to be.

I also thought about showing it as a comment instead without failing the build (IWYU does that) but that still doesn't solve the baseline. And not failing the build or having it disjoint will

I see it just like the results of any other test. If I change the output I need to adjust tests. I also feel this would make some things easier to track.

And I just realized that fail the builds on linting stuff like formatting. That has no impact on the behavior and we require immediate mitigation. You could argue that we just do a clean up step when the release is done - but since it is enforced in the CI we don't have to. It is kind of similar to the translations files - that should be kept in line so they are actually correct. But we have it disabled since there is currently too much noise from unrelated changes. That does not apply in this case since you only need to adjust the file if you actually did something to the ValueFlow. It does not just change because you fixed a random bug.

[firewave]

It could also help to expose gaps in the unit tests. The issue it found in the changes I did was not caught by those but unfortunately I was not able to reduce it into a test (yet - I will give it another try). Having that test might have helped to determine if the changes in #7827 accidentally introduced the same "issue" (I raised that but it was not acknowledged at all).

That would be preferable to get it to the customer/daca, check the validity, reduce the example and bail it down to an unintentional ValueFlow change.