I will try and demonstrate a general issue with a specific example.
Here is an example of some legacy C++ code which already has reasonably thorough (but verbose) error and exception handling:
bool Decompiler::decompile()
{
bool ok = true;
DIR *dir = nullptr;
try
{
dir = opendir(this->getRootDirectory().c_str());
if (dir != nullptr)
{
this->start();
ok = this->decompile(this->getRootDirectory(), dir) && ok;
this->end();
}
else
{
ok = false;
this->gotBadDirectory(this->getRootDirectory());
}
}
catch (...)
{
ok = false;
closedir(dir);
throw;
}
closedir(dir);
return ok;
}
Sonar rightly detected that closedir() should not be called with a nullptr (though it is probably harmless in practice).
So to correct this the simple and obvious way the code becomes:
bool Decompiler::decompile()
{
bool ok = true;
DIR *dir = nullptr;
try
{
dir = opendir(this->getRootDirectory().c_str());
if (dir != nullptr)
{
this->start();
ok = this->decompile(this->getRootDirectory(), dir) && ok;
this->end();
}
else
{
ok = false;
this->gotBadDirectory(this->getRootDirectory());
}
}
catch (...)
{
ok = false;
if (dir != nullptr)
{
closedir(dir);
}
throw;
}
if (dir != nullptr)
{
closedir(dir);
}
return ok;
}
So I remove the issue found.
But because I have resolved this issue there are new lines which must be covered.
The quality gate rejects the code as having insufficient coverage.
No current tests execute the catch block.
The kind of exception that this handles might be something like std::bad_alloc
Adding a test to induce an exception will result in even bigger changes.
It does not seem realistic to use dependency injection to replace every function call (for example the decompile() and gotBadDirectory() methods).
That would generate even more new code which will also need sufficient tests to meet the coverage goal.
I know there is an option to disable coverage of exceptional paths.
It would be a big mistake to disable it as coverage of exceptions is highly important in many cases.
I am strongly against this.
Now there are other ways to rewrite this to avoid problems. Make the directory an RAII resource for example.
But the original task was to fix a tiny defect found by sonar (with a estimate time of 1m) not the missing coverage of an obscure exceptional path.
The issue is that making even a small change potentially spirals into a much bigger job.
How can this be balanced effectively?
Or how can I fix one issue and defer fixing unrelated issues in nearby code?
How are other people approaching this balancing act?