Skip to content

Commit

Permalink
Merge pull request #117 from DaveLiddament/feature/clean-up
Browse files Browse the repository at this point in the history
Feature/clean up
  • Loading branch information
DaveLiddament authored Aug 7, 2022
2 parents 27667a9 + aebbd1c commit b363129
Show file tree
Hide file tree
Showing 10 changed files with 274 additions and 19 deletions.
17 changes: 13 additions & 4 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
* [Requirements](#requirements)
* [Installing](#installing)
* [Using SARB](#using-sarb)
* [Examples](#examples)
* [Further reading](#further-reading)

## Why SARB?
Expand All @@ -30,10 +29,10 @@ to legacy projects the tools have probably reported thousands of problems.
It's unrealistic to fix all but the most critical ones before continuing development.

SARB is used to create a baseline of these results. As work on the project
progresses SARB can takes the latest static analysis results, removes
progresses SARB takes the latest static analysis results, removes
those issues in the baseline and report the issues raised since the baseline.
SARB does this, in conjunction with git, by tracking lines of code between commits.
Currently SARB only supports git but it is possible to [add support for other SCMs](docs/NewHistoryAnalyser.md).
Currently, SARB only supports git, but it is possible to [add support for other SCMs](docs/NewHistoryAnalyser.md).

SARB is written in PHP, however it can be used to baseline results for any language and [any static analysis tool](docs/CustomInputFormats.md).

Expand All @@ -44,7 +43,7 @@ SARB should not be used on greenfield projects. If you're lucky enough to work o

## Requirements

Currently SARB only supports projects that use [git](https://git-scm.com/).
Currently, SARB only supports projects that use [git](https://git-scm.com/).

SARB requires PHP >= 7.3 to run. The project being analysed does not need to run PHP 7.3 or even be a PHP project at all.

Expand Down Expand Up @@ -189,6 +188,16 @@ To get the full history checked out use this:
Also don't forget to use the SARB option `--output-format=github`.
It will annotate your PR with any issues that have been added since the baseline.

## Gradually improving the codebase

In an ideal world SARB should not be required. SARB prevents you from adding new issues to your codebase.

It also provides a `--clean-up` option when running `remove`.
Running SARB with this option will pick out 5 random issues that are still in the baseline.
Challenge your team to fix 5 issues in the baseline every day.
Over a working year that'll be 1000 issues gone from the baseline!
Soon you'll be able to ditch SARB for good!


## Further Reading

Expand Down
17 changes: 10 additions & 7 deletions src/Domain/Pruner/PrunedResults.php
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,18 @@ class PrunedResults
*/
private $prunedResults;
/**
* @var int
* @var AnalysisResults
*/
private $inputAnalysisResultsCount;
private $inputAnalysisResults;

public function __construct(BaseLine $baseLine, AnalysisResults $prunedResults, int $inputAnalysisResultsCount)
{
public function __construct(
BaseLine $baseLine,
AnalysisResults $prunedResults,
AnalysisResults $inputAnalysisResults
) {
$this->baseLine = $baseLine;
$this->prunedResults = $prunedResults;
$this->inputAnalysisResultsCount = $inputAnalysisResultsCount;
$this->inputAnalysisResults = $inputAnalysisResults;
}

public function getBaseLine(): BaseLine
Expand All @@ -39,8 +42,8 @@ public function getPrunedResults(): AnalysisResults
return $this->prunedResults;
}

public function getInputAnalysisResultsCount(): int
public function getInputAnalysisResults(): AnalysisResults
{
return $this->inputAnalysisResultsCount;
return $this->inputAnalysisResults;
}
}
3 changes: 1 addition & 2 deletions src/Domain/Pruner/ResultsPruner.php
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ class ResultsPruner implements ResultsPrunerInterface
* @var BaseLineImporter
*/
private $baseLineImporter;

/**
* @var BaseLineResultsRemover
*/
Expand Down Expand Up @@ -64,6 +63,6 @@ public function getPrunedResults(
$baseLine->getAnalysisResults()
);

return new PrunedResults($baseLine, $outputAnalysisResults, $inputAnalysisResults->getCount());
return new PrunedResults($baseLine, $outputAnalysisResults, $inputAnalysisResults);
}
}
45 changes: 45 additions & 0 deletions src/Domain/RandomResultsPicker/RandomResultsPicker.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
<?php

declare(strict_types=1);

namespace DaveLiddament\StaticAnalysisResultsBaseliner\Domain\RandomResultsPicker;

use DaveLiddament\StaticAnalysisResultsBaseliner\Domain\ResultsParser\AnalysisResults;
use DaveLiddament\StaticAnalysisResultsBaseliner\Domain\ResultsParser\AnalysisResultsBuilder;
use DaveLiddament\StaticAnalysisResultsBaseliner\Domain\Utils\RandomNumberGenerator;

class RandomResultsPicker
{
private const RANDOM_ISSUES_TO_FIX = 5;

/**
* @var RandomNumberGenerator
*/
private $randomNumberGenerator;

public function __construct(RandomNumberGenerator $randomNumberGenerator)
{
$this->randomNumberGenerator = $randomNumberGenerator;
}

/**
* Returns a random selection of the issues found.
*/
public function getRandomResultsToFix(AnalysisResults $issues): AnalysisResults
{
$randomIssuesToFix = min(self::RANDOM_ISSUES_TO_FIX, $issues->getCount());

$randomIssues = new AnalysisResultsBuilder();
$issuesToPickFrom = $issues->getAnalysisResults();

for ($i = 0; $i < $randomIssuesToFix; ++$i) {
$totalRemaining = count($issuesToPickFrom);
$issuePickedIndex = $this->randomNumberGenerator->getRandomNumber($totalRemaining - 1);
$issuePicked = $issuesToPickFrom[$issuePickedIndex];
$randomIssues->addAnalysisResult($issuePicked);
array_splice($issuesToPickFrom, $issuePickedIndex, 1);
}

return $randomIssues->build();
}
}
16 changes: 16 additions & 0 deletions src/Domain/Utils/RandomNumberGenerator.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
<?php

declare(strict_types=1);

namespace DaveLiddament\StaticAnalysisResultsBaseliner\Domain\Utils;

/**
* Wrapper for rand.
*/
class RandomNumberGenerator
{
public function getRandomNumber(int $maxNumber): int
{
return rand(0, $maxNumber);
}
}
48 changes: 45 additions & 3 deletions src/Framework/Command/RemoveBaseLineFromResultsCommand.php
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
use DaveLiddament\StaticAnalysisResultsBaseliner\Domain\OutputFormatter\OutputFormatter;
use DaveLiddament\StaticAnalysisResultsBaseliner\Domain\OutputFormatter\OutputFormatterLookupService;
use DaveLiddament\StaticAnalysisResultsBaseliner\Domain\Pruner\ResultsPrunerInterface;
use DaveLiddament\StaticAnalysisResultsBaseliner\Domain\RandomResultsPicker\RandomResultsPicker;
use DaveLiddament\StaticAnalysisResultsBaseliner\Framework\Command\internal\BaseLineFileHelper;
use DaveLiddament\StaticAnalysisResultsBaseliner\Framework\Command\internal\CliConfigReader;
use DaveLiddament\StaticAnalysisResultsBaseliner\Framework\Command\internal\ErrorReporter;
Expand All @@ -34,6 +35,7 @@ class RemoveBaseLineFromResultsCommand extends Command
public const COMMAND_NAME = 'remove-baseline-results';

private const OUTPUT_FORMAT = 'output-format';
private const SHOW_RANDOM_ERRORS = 'clean-up';

/**
* @var string|null
Expand All @@ -48,14 +50,26 @@ class RemoveBaseLineFromResultsCommand extends Command
* @var ResultsPrunerInterface
*/
private $resultsPruner;
/**
* @var TableOutputFormatter
*/
private $tableOutputFormatter;
/**
* @var RandomResultsPicker
*/
private $randomResultsPicker;

public function __construct(
ResultsPrunerInterface $resultsPruner,
OutputFormatterLookupService $outputFormatterLookupService
OutputFormatterLookupService $outputFormatterLookupService,
TableOutputFormatter $tableOutputFormatter,
RandomResultsPicker $randomResultsPicker
) {
$this->outputFormatterLookupService = $outputFormatterLookupService;
parent::__construct(self::COMMAND_NAME);
$this->resultsPruner = $resultsPruner;
$this->tableOutputFormatter = $tableOutputFormatter;
$this->randomResultsPicker = $randomResultsPicker;
}

protected function configure(): void
Expand All @@ -71,6 +85,13 @@ protected function configure(): void
TableOutputFormatter::CODE
);

$this->addOption(
self::SHOW_RANDOM_ERRORS,
null,
InputOption::VALUE_NONE,
'Show a random 5 issues in the baseline to fix'
);

ProjectRootHelper::configureProjectRootOption($this);

BaseLineFileHelper::configureBaseLineFileArgument($this);
Expand All @@ -83,6 +104,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int
$outputFormatter = $this->getOutputFormatter($input);
$baseLineFileName = BaseLineFileHelper::getBaselineFile($input);
$inputAnalysisResultsAsString = CliConfigReader::getStdin($input);
$showRandomIssues = CliConfigReader::getBooleanOption($input, self::SHOW_RANDOM_ERRORS);

$prunedResults = $this->resultsPruner->getPrunedResults(
$baseLineFileName,
Expand All @@ -94,7 +116,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int

OutputWriter::writeToStdError(
$output,
"Latest analysis issue count: {$prunedResults->getInputAnalysisResultsCount()}",
"Latest analysis issue count: {$prunedResults->getInputAnalysisResults()->getCount()}",
false
);

Expand All @@ -113,7 +135,27 @@ protected function execute(InputInterface $input, OutputInterface $output): int
$outputAsString = $outputFormatter->outputResults($outputAnalysisResults);
$output->writeln($outputAsString);

return $outputAnalysisResults->hasNoIssues() ? 0 : 1;
$returnCode = $outputAnalysisResults->hasNoIssues() ? 0 : 1;

if ($showRandomIssues && !$prunedResults->getInputAnalysisResults()->hasNoIssues()) {
$randomIssues = $this->randomResultsPicker->getRandomResultsToFix($prunedResults->getInputAnalysisResults());

OutputWriter::writeToStdError(
$output,
"\n\nRandom {$randomIssues->getCount()} issues in the baseline to fix...",
false
);

$outputAsString = $this->tableOutputFormatter->outputResults($randomIssues);

OutputWriter::writeToStdError(
$output,
$outputAsString,
false
);
}

return $returnCode;
} catch (Throwable $throwable) {
$returnCode = ErrorReporter::reportError($output, $throwable);

Expand Down
1 change: 1 addition & 0 deletions src/Framework/Command/internal/ErrorReporter.php
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ public static function reportError(OutputInterface $output, Throwable $throwable
} catch (Throwable $e) {
// This should never happen. All exceptions should extend SarbException
OutputWriter::writeToStdError($output, "Unexpected critical error: {$e->getMessage()}", true);
OutputWriter::writeToStdError($output, $e->getTraceAsString(), true);

return 100;
}
Expand Down
2 changes: 1 addition & 1 deletion src/Plugins/OutputFormatters/TableOutputFormatter.php
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ private function addIssuesInTable(BufferedOutput $output, AnalysisResults $analy
$currentTable->setHeaders($headings);
}

Assert::notNull($currentTable);
Assert::notNull($currentTable, 'No Table object');
$currentTable->addRow([
$analysisResult->getLocation()->getLineNumber()->getLineNumber(),
$analysisResult->getMessage(),
Expand Down
114 changes: 114 additions & 0 deletions tests/Unit/Core/RandomResultsPicker/RandomResultsPickerTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
<?php

declare(strict_types=1);

namespace DaveLiddament\StaticAnalysisResultsBaseliner\Tests\Unit\Core\RandomResultsPicker;

use DaveLiddament\StaticAnalysisResultsBaseliner\Domain\Common\LineNumber;
use DaveLiddament\StaticAnalysisResultsBaseliner\Domain\Common\Location;
use DaveLiddament\StaticAnalysisResultsBaseliner\Domain\Common\ProjectRoot;
use DaveLiddament\StaticAnalysisResultsBaseliner\Domain\Common\RelativeFileName;
use DaveLiddament\StaticAnalysisResultsBaseliner\Domain\Common\Type;
use DaveLiddament\StaticAnalysisResultsBaseliner\Domain\RandomResultsPicker\RandomResultsPicker;
use DaveLiddament\StaticAnalysisResultsBaseliner\Domain\ResultsParser\AnalysisResult;
use DaveLiddament\StaticAnalysisResultsBaseliner\Domain\ResultsParser\AnalysisResultsBuilder;
use DaveLiddament\StaticAnalysisResultsBaseliner\Domain\Utils\RandomNumberGenerator;
use PHPUnit\Framework\MockObject\Stub;
use PHPUnit\Framework\TestCase;

class RandomResultsPickerTest extends TestCase
{
/**
* @var ProjectRoot
*/
private $projectRoot;
/**
* @var RandomResultsPicker
*/
private $randomResultsPicker;
/**
* @var RandomNumberGenerator&Stub
*/
private $randomNumberGenerator;

protected function setUp(): void
{
$this->projectRoot = ProjectRoot::fromCurrentWorkingDirectory('/');

$this->randomNumberGenerator = $this->createStub(RandomNumberGenerator::class);
$this->randomResultsPicker = new RandomResultsPicker($this->randomNumberGenerator);
}

public function testPick5Results(): void
{
$issuesBuilder = new AnalysisResultsBuilder();
$issue1 = $this->addIssue($issuesBuilder, 'a');
$issue2 = $this->addIssue($issuesBuilder, 'b');
$this->addIssue($issuesBuilder, 'c');
$issue4 = $this->addIssue($issuesBuilder, 'd');
$this->addIssue($issuesBuilder, 'e');
$issue6 = $this->addIssue($issuesBuilder, 'f');
$issue7 = $this->addIssue($issuesBuilder, 'g');

$this->setRandomNumbers(5, 1, 0, 3, 1);

$pickedIssues = $this->randomResultsPicker->getRandomResultsToFix($issuesBuilder->build());

$expected = [
$issue1,
$issue2,
$issue4,
$issue6,
$issue7,
];

$this->assertEquals($expected, $pickedIssues->getAnalysisResults());
$this->assertSame(5, $pickedIssues->getCount());
$this->assertFalse($pickedIssues->hasNoIssues());
}

public function testPickWhenFewerThan5Results(): void
{
$issuesBuilder = new AnalysisResultsBuilder();
$issue1 = $this->addIssue($issuesBuilder, 'a');
$issue2 = $this->addIssue($issuesBuilder, 'b');
$issue3 = $this->addIssue($issuesBuilder, 'c');

$this->setRandomNumbers(2, 0, 0);

$pickedIssues = $this->randomResultsPicker->getRandomResultsToFix($issuesBuilder->build());

$expected = [
$issue1,
$issue2,
$issue3,
];

$this->assertEquals($expected, $pickedIssues->getAnalysisResults());
$this->assertSame(3, $pickedIssues->getCount());
$this->assertFalse($pickedIssues->hasNoIssues());
}

private function addIssue(AnalysisResultsBuilder $issuesBuilder, string $string): AnalysisResult
{
$analysisResult = new AnalysisResult(
Location::fromRelativeFileName(
new RelativeFileName($string),
$this->projectRoot,
new LineNumber(10),
),
new Type("Type-{$string}"),
$string,
[]
);

$issuesBuilder->addAnalysisResult($analysisResult);

return $analysisResult;
}

private function setRandomNumbers(int ...$numbers): void
{
$this->randomNumberGenerator->method('getRandomNumber')->willReturnOnConsecutiveCalls(...$numbers);
}
}
Loading

0 comments on commit b363129

Please sign in to comment.