Update tooManyConfigsError error message to prevent duplication in error message

[GNL10]

No description provided.

[firewave]

Please add a test.

[firewave]

Thanks but a unit test does not seem to make sense here as it is not clear how actual inputs can trigger this behavior.

Also the unit test just calls the function triggering the error should might not represent as it actually occurs in the executors. So it might be tested cases which will never occur.

[GNL10]

Thanks but a unit test does not seem to make sense here as it is not clear how actual inputs can trigger this behavior.

Also the unit test just calls the function triggering the error should might not represent as it actually occurs in the executors. So it might be tested cases which will never occur.

You are right, but looking at it, the only place where it would make sense to test that empty file, would be the getErrorMessages UT.
But this UT results in 333 messages in errorLogger.errmsgs (at the current commit). The second position in the iterator of errorLogger.errmsgs would be the one where we could apply the validation. But it looks strange validating only one position, and the second one at that.

I added an update, so you can see how it would look like. But I, myself, am not too happy about how it looks like.

[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]

This was not caused by you. But the verbose message does not say how many configurations are checked or what the maxConfigs options is and that is unfortunate.

[danmar]

Thanks but a unit test does not seem to make sense here as it is not clear how actual inputs can trigger this behavior.

imho the idea of unit tests is to test each function individually. And I like to unit test functions.

Also the unit test just calls the function triggering the error should might not represent as it actually occurs in the executors. So it might be tested cases which will never occur.

well it would be interesting to know if this can be reproduced. Spontanously I don't understand why this function would ever be called when numberOfConfigurations is <= maxConfigs .. can you @GNL10 share some info about this?

[GNL10]

Thanks but a unit test does not seem to make sense here as it is not clear how actual inputs can trigger this behavior.

imho the idea of unit tests is to test each function individually. And I like to unit test functions.

Also the unit test just calls the function triggering the error should might not represent as it actually occurs in the executors. So it might be tested cases which will never occur.

well it would be interesting to know if this can be reproduced. Spontanously I don't understand why this function would ever be called when numberOfConfigurations is <= maxConfigs .. can you @GNL10 share some info about this?

The function is called in 3 different places.

In CppCheck::checkInternal:

if (!mSettings.force && configurations.size() > mSettings.maxConfigs) {
            if (mSettings.severity.isEnabled(Severity::information)) {
                tooManyConfigsError(Path::toNativeSeparators(file.spath()),configurations.size());

In CppCheckExecutor::check_internal:

if (!settings.checkConfiguration) {
        cppcheck.tooManyConfigsError("",0U);

Another in CppCheck::getErrorMessages:

cppcheck.mTooManyConfigs = true;
cppcheck.tooManyConfigsError("",0U);

So these 2 last ones seem to force the tooManyConfigsError, one explicitly forces it via cppcheck.mTooManyConfigs = true; and both set the file variable as empty, to force the error.

[danmar]

In CppCheck::checkInternal:

ok that looks good.

Another in CppCheck::getErrorMessages:

I think it would be better to write the same error message that is written in CppCheck::checkInternal

In CppCheckExecutor::check_internal:

For information. The code looks questionable. I am not so sure that I want to have this printout. I believe my original idea for this was that if there was more configurations than maxConfigs in one file and we didn't write such messages directly then we would at least write a message at the end.

I feel that if users wants to see these messages they should just turn on information.

EDIT: I would suggest that we ignore the call from CppCheckExecutor::check_internal it should be removed (it can be removed in a separate PR). Please tweak the tooManyConfigs message so it always writes how many configurations are checked and maxConfigs.

[danmar]

I would suggest that we ignore the call from CppCheckExecutor::check_internal it should be removed (it can be removed in a separate PR)

if you remove this then it would seem to me that you solve https://trac.cppcheck.net/ticket/13825

[danmar]

As https://trac.cppcheck.net/ticket/14167 shows the code in CppCheck::checkInternal is not perfect, a tweak for that would be nice.

EDIT: sorry I got carried away 14167 feels out-of-scope anyway even though it is related. It is acceptable if you keep this PR small and simple.. and we can fix 14167 later..

[firewave]

I still would like to see a code snippet which produces the error via the CLI.