From f70896324669a7e22b3de125f05adb3e7df0cc35 Mon Sep 17 00:00:00 2001 From: Lars Moelleken Date: Sun, 26 Jul 2026 01:07:15 +0200 Subject: [PATCH 1/5] Add fixable native type sniff --- docbookcs.xml.dist | 2 + src/Fix/Fixer/NativeTypeFixer.php | 35 +++++ src/Sniff/NativeTypeSniff.php | 190 +++++++++++++++++++++++ tests/Unit/Fix/NativeTypeFixerTest.php | 78 ++++++++++ tests/Unit/Sniff/NativeTypeSniffTest.php | 105 +++++++++++++ 5 files changed, 410 insertions(+) create mode 100644 src/Fix/Fixer/NativeTypeFixer.php create mode 100644 src/Sniff/NativeTypeSniff.php create mode 100644 tests/Unit/Fix/NativeTypeFixerTest.php create mode 100644 tests/Unit/Sniff/NativeTypeSniffTest.php diff --git a/docbookcs.xml.dist b/docbookcs.xml.dist index 51ee17a..910c84b 100644 --- a/docbookcs.xml.dist +++ b/docbookcs.xml.dist @@ -16,6 +16,8 @@ + + diff --git a/src/Fix/Fixer/NativeTypeFixer.php b/src/Fix/Fixer/NativeTypeFixer.php new file mode 100644 index 0000000..9ac3a29 --- /dev/null +++ b/src/Fix/Fixer/NativeTypeFixer.php @@ -0,0 +1,35 @@ +rangeOne(); + if ($range->content === null) { + throw FixerException::cannotFixMissingContent(); + } + + if ($violation->message === 'A union containing mixed is redundant and should be mixed.') { + if (preg_match('/^]*\bclass\s*=\s*(["\'])union\1[^>]*>.*<\/type>$/is', $range->content) !== 1) { + throw FixerException::cannotFixInvalidContent($violation); + } + + return Fix::fromViolationAndRange($violation, $range, 'mixed'); + } + + if (preg_match('/should be written as "([a-z]+)"\.$/', $violation->message, $matches) !== 1) { + throw FixerException::cannotFixInvalidContent($violation); + } + + return Fix::fromViolationAndRange($violation, $range, $matches[1]); + } +} diff --git a/src/Sniff/NativeTypeSniff.php b/src/Sniff/NativeTypeSniff.php new file mode 100644 index 0000000..9be3b8a --- /dev/null +++ b/src/Sniff/NativeTypeSniff.php @@ -0,0 +1,190 @@ + 'bool', + 'double' => 'float', + 'integer' => 'int', + 'real' => 'float', + ]; + + public static function getCode(): string + { + return 'DocbookCS.NativeType'; + } + + public static function getFixerClassName(): string + { + return NativeTypeFixer::class; + } + + /** + * @throws \InvalidArgumentException if a generated source range is inconsistent + * @throws \OutOfBoundsException if a matched type lies outside the source + */ + public function process(\DOMDocument $document, File $file): array + { + $source = $this->maskNonElementMarkup($file->content); + $synopsisRanges = $this->synopsisRanges($source); + $violations = []; + + preg_match_all('/]*>([^<]*)<\/type>/i', $source, $matches, PREG_OFFSET_CAPTURE); + foreach ($matches[1] as [$typeName, $offset]) { + $offset = (int) $offset; + if (!$this->isInsideSynopsis($offset, $synopsisRanges)) { + continue; + } + + $canonical = $this->canonicalType(trim($typeName)); + if ($canonical === null || $canonical === trim($typeName)) { + continue; + } + + $leadingWhitespace = strlen($typeName) - strlen(ltrim($typeName)); + $beginOffset = $offset + $leadingWhitespace; + $violations[] = $this->createViolation( + $file->path, + sprintf('Native type "%s" should be written as "%s".', trim($typeName), $canonical), + [SourceRange::fromFile($file, $beginOffset, $beginOffset + strlen(trim($typeName)))], + ); + } + + foreach ($this->unionMatches($source) as $union) { + if (!$this->isInsideSynopsis($union['beginOffset'], $synopsisRanges)) { + continue; + } + + $members = array_map('strtolower', $union['members']); + if (count($members) < 2 || !in_array('mixed', $members, true)) { + continue; + } + + $violations[] = $this->createViolation( + $file->path, + 'A union containing mixed is redundant and should be mixed.', + [SourceRange::fromFile($file, $union['beginOffset'], $union['untilOffset'])], + ); + } + + usort( + $violations, + static fn(\DocbookCS\Violation\Violation $a, \DocbookCS\Violation\Violation $b): int => + $a->rangeOne()->beginOffset <=> $b->rangeOne()->beginOffset, + ); + + return $violations; + } + + private function canonicalType(string $typeName): ?string + { + $lowercase = strtolower($typeName); + + return self::TYPE_ALIASES[$lowercase] + ?? (in_array($lowercase, self::NATIVE_TYPES, true) ? $lowercase : null); + } + + /** @return list */ + private function synopsisRanges(string $source): array + { + preg_match_all('/<\/?(?:method|constructor)synopsis\b[^>]*>/i', $source, $matches, PREG_OFFSET_CAPTURE); + $stack = []; + $ranges = []; + foreach ($matches[0] as [$tag, $offset]) { + $offset = (int) $offset; + if (!str_starts_with($tag, ' $beginOffset, 'untilOffset' => $offset + strlen($tag)]; + } + } + + return $ranges; + } + + /** @param list $ranges */ + private function isInsideSynopsis(int $offset, array $ranges): bool + { + foreach ($ranges as $range) { + if ($offset >= $range['beginOffset'] && $offset < $range['untilOffset']) { + return true; + } + } + + return false; + } + + /** + * @return list}> + */ + private function unionMatches(string $source): array + { + preg_match_all('/<\/?type\b[^>]*>/i', $source, $matches, PREG_OFFSET_CAPTURE); + /** @var list}> $stack */ + $stack = []; + $unions = []; + + foreach ($matches[0] as [$tag, $offset]) { + $offset = (int) $offset; + if (!str_starts_with($tag, '')) { + if ($stack !== [] && $stack[array_key_last($stack)]['union']) { + $stack[array_key_last($stack)]['members'][] = ''; + } + continue; + } + + $stack[] = [ + 'beginOffset' => $offset, + 'contentOffset' => $offset + strlen($tag), + 'union' => preg_match('/\bclass\s*=\s*(["\'])union\1/i', $tag) === 1, + 'members' => [], + ]; + continue; + } + + if (null === $type = array_pop($stack)) { + continue; + } + + $content = trim(substr($source, $type['contentOffset'], $offset - $type['contentOffset'])); + if ($type['union']) { + $unions[] = [ + 'beginOffset' => $type['beginOffset'], + 'untilOffset' => $offset + strlen($tag), + 'members' => $type['members'], + ]; + } elseif ($stack !== [] && $stack[array_key_last($stack)]['union']) { + $stack[array_key_last($stack)]['members'][] = $content; + } + } + + return $unions; + } +} diff --git a/tests/Unit/Fix/NativeTypeFixerTest.php b/tests/Unit/Fix/NativeTypeFixerTest.php new file mode 100644 index 0000000..4ad2338 --- /dev/null +++ b/tests/Unit/Fix/NativeTypeFixerTest.php @@ -0,0 +1,78 @@ +Arrayexample' + . 'integervalue'; + + $result = $this->fix($content); + + self::assertSame( + 'arrayexample' + . 'intvalue', + $result->file->content, + ); + self::assertSame(2, $result->applied); + } + + #[Test] + public function itCollapsesAUnionContainingMixed(): void + { + $content = 'mixednull' + . 'example'; + + $result = $this->fix($content); + + self::assertSame( + 'mixedexample', + $result->file->content, + ); + self::assertSame(1, $result->applied); + } + + private function fix(string $content): FixResult + { + $file = new File('file.xml', $content); + $document = new \DOMDocument(); + $document->loadXML($content); + $violations = new NativeTypeSniff(RunMode::Fix)->process($document, $file); + $fixer = new NativeTypeFixer(); + $fixes = array_map($fixer->process(...), $violations); + + return new FixApplier()->apply($file, $fixes); + } +} diff --git a/tests/Unit/Sniff/NativeTypeSniffTest.php b/tests/Unit/Sniff/NativeTypeSniffTest.php new file mode 100644 index 0000000..08ed0c2 --- /dev/null +++ b/tests/Unit/Sniff/NativeTypeSniffTest.php @@ -0,0 +1,105 @@ +stringexample' + . 'ExampleClassvalue'; + + self::assertSame([], $this->process($content)); + } + + #[Test] + public function itReportsNonCanonicalCasingAndAliases(): void + { + $content = 'Arrayexample' + . 'integervalue' + . 'BOOLEANenabled'; + + $violations = $this->process($content); + + self::assertCount(3, $violations); + self::assertSame('array', $this->replacementFromMessage($violations[0]->message)); + self::assertSame('int', $this->replacementFromMessage($violations[1]->message)); + self::assertSame('bool', $this->replacementFromMessage($violations[2]->message)); + self::assertSame('Array', $violations[0]->rangeOne()->content); + } + + #[Test] + public function itReportsMixedCombinedWithAnotherUnionMember(): void + { + $content = 'mixednull' + . 'example'; + + $violations = $this->process($content); + + self::assertCount(1, $violations); + self::assertStringContainsString('union containing mixed', $violations[0]->message); + self::assertSame( + 'mixednull', + $violations[0]->rangeOne()->content, + ); + } + + #[Test] + public function itDoesNotInspectTypeElementsOutsideSynopsesOrInsideComments(): void + { + $content = 'String' + . '' + . 'voidexample'; + + self::assertSame([], $this->process($content)); + } + + #[Test] + public function itReportsInSourceOrder(): void + { + $content = "\n Stringexample\n" + . " Callablevalue\n"; + + $violations = $this->process($content); + + self::assertCount(2, $violations); + self::assertSame(2, $violations[0]->rangeOne()->line); + self::assertSame(3, $violations[1]->rangeOne()->line); + } + + /** @return list */ + private function process(string $content): array + { + $document = new \DOMDocument(); + $document->loadXML($content); + + return new NativeTypeSniff()->process($document, new File('file.xml', $content)); + } + + private function replacementFromMessage(string $message): string + { + self::assertSame(1, preg_match('/should be written as "([a-z]+)"/', $message, $matches)); + + return $matches[1]; + } +} From 3e78b13e1993c59436b5496728c410b3036d2283 Mon Sep 17 00:00:00 2001 From: Lars Moelleken Date: Sun, 26 Jul 2026 01:28:24 +0200 Subject: [PATCH 2/5] Prevent overlapping mixed union fixes --- docbookcs.xml.dist | 2 + src/Fix/Fixer/MixedUnionFixer.php | 30 +++++++++ src/Fix/Fixer/NativeTypeFixer.php | 12 +--- src/Sniff/AbstractSniff.php | 9 ++- src/Sniff/MixedUnionDetector.php | 56 +++++++++++++++++ src/Sniff/MixedUnionSniff.php | 78 +++++++++++++++++++++++ src/Sniff/NativeTypeSniff.php | 79 +++++------------------- src/Violation/Violation.php | 1 + tests/Unit/Fix/MixedUnionFixerTest.php | 64 +++++++++++++++++++ tests/Unit/Fix/NativeTypeFixerTest.php | 17 +---- tests/Unit/Sniff/MixedUnionSniffTest.php | 75 ++++++++++++++++++++++ tests/Unit/Sniff/NativeTypeSniffTest.php | 16 ++--- 12 files changed, 338 insertions(+), 101 deletions(-) create mode 100644 src/Fix/Fixer/MixedUnionFixer.php create mode 100644 src/Sniff/MixedUnionDetector.php create mode 100644 src/Sniff/MixedUnionSniff.php create mode 100644 tests/Unit/Fix/MixedUnionFixerTest.php create mode 100644 tests/Unit/Sniff/MixedUnionSniffTest.php diff --git a/docbookcs.xml.dist b/docbookcs.xml.dist index 910c84b..e4c3370 100644 --- a/docbookcs.xml.dist +++ b/docbookcs.xml.dist @@ -18,6 +18,8 @@ + + diff --git a/src/Fix/Fixer/MixedUnionFixer.php b/src/Fix/Fixer/MixedUnionFixer.php new file mode 100644 index 0000000..c289532 --- /dev/null +++ b/src/Fix/Fixer/MixedUnionFixer.php @@ -0,0 +1,30 @@ +rangeOne(); + if ($range->content === null) { + throw FixerException::cannotFixMissingContent(); + } + + if ( + $violation->replacement !== 'mixed' + || preg_match('/^]*\bclass\s*=\s*(["\'])union\1[^>]*>.*<\/type>$/is', $range->content) !== 1 + ) { + throw FixerException::cannotFixInvalidContent($violation); + } + + return Fix::fromViolationAndRange($violation, $range, $violation->replacement); + } +} diff --git a/src/Fix/Fixer/NativeTypeFixer.php b/src/Fix/Fixer/NativeTypeFixer.php index 9ac3a29..c891867 100644 --- a/src/Fix/Fixer/NativeTypeFixer.php +++ b/src/Fix/Fixer/NativeTypeFixer.php @@ -18,18 +18,10 @@ public function process(Violation $violation): Fix throw FixerException::cannotFixMissingContent(); } - if ($violation->message === 'A union containing mixed is redundant and should be mixed.') { - if (preg_match('/^]*\bclass\s*=\s*(["\'])union\1[^>]*>.*<\/type>$/is', $range->content) !== 1) { - throw FixerException::cannotFixInvalidContent($violation); - } - - return Fix::fromViolationAndRange($violation, $range, 'mixed'); - } - - if (preg_match('/should be written as "([a-z]+)"\.$/', $violation->message, $matches) !== 1) { + if ($violation->replacement === null || preg_match('/^[a-z]+$/', $violation->replacement) !== 1) { throw FixerException::cannotFixInvalidContent($violation); } - return Fix::fromViolationAndRange($violation, $range, $matches[1]); + return Fix::fromViolationAndRange($violation, $range, $violation->replacement); } } diff --git a/src/Sniff/AbstractSniff.php b/src/Sniff/AbstractSniff.php index aed3eca..9d88550 100644 --- a/src/Sniff/AbstractSniff.php +++ b/src/Sniff/AbstractSniff.php @@ -87,14 +87,19 @@ protected function elementNameRanges(File $file, int $beginOffset, int $untilOff * * @throws \InvalidArgumentException if the affected ranges are inconsistent */ - protected function createViolation(string $filePath, string $message, array $affectedRanges): Violation - { + protected function createViolation( + string $filePath, + string $message, + array $affectedRanges, + ?string $replacement = null, + ): Violation { return new Violation( sniffCode: static::getCode(), filePath: $filePath, message: $message, affectedRanges: $affectedRanges, severity: $this->severity, + replacement: $replacement, ); } diff --git a/src/Sniff/MixedUnionDetector.php b/src/Sniff/MixedUnionDetector.php new file mode 100644 index 0000000..d996ec1 --- /dev/null +++ b/src/Sniff/MixedUnionDetector.php @@ -0,0 +1,56 @@ + */ + public static function matches(string $source): array + { + preg_match_all('/<\/?type\b[^>]*>/i', $source, $matches, PREG_OFFSET_CAPTURE); + /** @var list}> $stack */ + $stack = []; + $unions = []; + + foreach ($matches[0] as [$tag, $offset]) { + $offset = (int) $offset; + if (!str_starts_with($tag, '')) { + if ($stack !== [] && $stack[array_key_last($stack)]['union']) { + $stack[array_key_last($stack)]['members'][] = ''; + } + continue; + } + + $stack[] = [ + 'beginOffset' => $offset, + 'contentOffset' => $offset + strlen($tag), + 'union' => preg_match('/\bclass\s*=\s*(["\'])union\1/i', $tag) === 1, + 'members' => [], + ]; + continue; + } + + if (null === $type = array_pop($stack)) { + continue; + } + + $content = trim(substr($source, $type['contentOffset'], $offset - $type['contentOffset'])); + if ($type['union']) { + $members = array_map('strtolower', $type['members']); + if (count($members) >= 2 && in_array('mixed', $members, true)) { + $unions[] = [ + 'beginOffset' => $type['beginOffset'], + 'untilOffset' => $offset + strlen($tag), + ]; + } + } elseif ($stack !== [] && $stack[array_key_last($stack)]['union']) { + $stack[array_key_last($stack)]['members'][] = $content; + } + } + + return $unions; + } +} diff --git a/src/Sniff/MixedUnionSniff.php b/src/Sniff/MixedUnionSniff.php new file mode 100644 index 0000000..4321aae --- /dev/null +++ b/src/Sniff/MixedUnionSniff.php @@ -0,0 +1,78 @@ +maskNonElementMarkup($file->content); + $synopsisRanges = $this->synopsisRanges($source); + $violations = []; + + foreach (MixedUnionDetector::matches($source) as $union) { + if (!$this->isInsideSynopsis($union['beginOffset'], $synopsisRanges)) { + continue; + } + + $violations[] = $this->createViolation( + $file->path, + 'A union containing mixed is redundant and should be mixed.', + [SourceRange::fromFile($file, $union['beginOffset'], $union['untilOffset'])], + 'mixed', + ); + } + + return $violations; + } + + /** @return list */ + private function synopsisRanges(string $source): array + { + preg_match_all('/<\/?(?:method|constructor)synopsis\b[^>]*>/i', $source, $matches, PREG_OFFSET_CAPTURE); + $stack = []; + $ranges = []; + foreach ($matches[0] as [$tag, $offset]) { + $offset = (int) $offset; + if (!str_starts_with($tag, ' $beginOffset, 'untilOffset' => $offset + strlen($tag)]; + } + } + + return $ranges; + } + + /** @param list $ranges */ + private function isInsideSynopsis(int $offset, array $ranges): bool + { + foreach ($ranges as $range) { + if ($offset >= $range['beginOffset'] && $offset < $range['untilOffset']) { + return true; + } + } + + return false; + } +} diff --git a/src/Sniff/NativeTypeSniff.php b/src/Sniff/NativeTypeSniff.php index 9be3b8a..e141caf 100644 --- a/src/Sniff/NativeTypeSniff.php +++ b/src/Sniff/NativeTypeSniff.php @@ -53,6 +53,10 @@ public function process(\DOMDocument $document, File $file): array { $source = $this->maskNonElementMarkup($file->content); $synopsisRanges = $this->synopsisRanges($source); + $redundantMixedUnions = array_values(array_filter( + MixedUnionDetector::matches($source), + fn(array $union): bool => $this->isInsideSynopsis($union['beginOffset'], $synopsisRanges), + )); $violations = []; preg_match_all('/]*>([^<]*)<\/type>/i', $source, $matches, PREG_OFFSET_CAPTURE); @@ -69,36 +73,19 @@ public function process(\DOMDocument $document, File $file): array $leadingWhitespace = strlen($typeName) - strlen(ltrim($typeName)); $beginOffset = $offset + $leadingWhitespace; - $violations[] = $this->createViolation( - $file->path, - sprintf('Native type "%s" should be written as "%s".', trim($typeName), $canonical), - [SourceRange::fromFile($file, $beginOffset, $beginOffset + strlen(trim($typeName)))], - ); - } - - foreach ($this->unionMatches($source) as $union) { - if (!$this->isInsideSynopsis($union['beginOffset'], $synopsisRanges)) { - continue; - } - - $members = array_map('strtolower', $union['members']); - if (count($members) < 2 || !in_array('mixed', $members, true)) { + $untilOffset = $beginOffset + strlen(trim($typeName)); + if ($this->isInsideUnion($beginOffset, $untilOffset, $redundantMixedUnions)) { continue; } $violations[] = $this->createViolation( $file->path, - 'A union containing mixed is redundant and should be mixed.', - [SourceRange::fromFile($file, $union['beginOffset'], $union['untilOffset'])], + sprintf('Native type "%s" should be written as "%s".', trim($typeName), $canonical), + [SourceRange::fromFile($file, $beginOffset, $untilOffset)], + $canonical, ); } - usort( - $violations, - static fn(\DocbookCS\Violation\Violation $a, \DocbookCS\Violation\Violation $b): int => - $a->rangeOne()->beginOffset <=> $b->rangeOne()->beginOffset, - ); - return $violations; } @@ -140,51 +127,15 @@ private function isInsideSynopsis(int $offset, array $ranges): bool return false; } - /** - * @return list}> - */ - private function unionMatches(string $source): array + /** @param list $unions */ + private function isInsideUnion(int $beginOffset, int $untilOffset, array $unions): bool { - preg_match_all('/<\/?type\b[^>]*>/i', $source, $matches, PREG_OFFSET_CAPTURE); - /** @var list}> $stack */ - $stack = []; - $unions = []; - - foreach ($matches[0] as [$tag, $offset]) { - $offset = (int) $offset; - if (!str_starts_with($tag, '')) { - if ($stack !== [] && $stack[array_key_last($stack)]['union']) { - $stack[array_key_last($stack)]['members'][] = ''; - } - continue; - } - - $stack[] = [ - 'beginOffset' => $offset, - 'contentOffset' => $offset + strlen($tag), - 'union' => preg_match('/\bclass\s*=\s*(["\'])union\1/i', $tag) === 1, - 'members' => [], - ]; - continue; - } - - if (null === $type = array_pop($stack)) { - continue; - } - - $content = trim(substr($source, $type['contentOffset'], $offset - $type['contentOffset'])); - if ($type['union']) { - $unions[] = [ - 'beginOffset' => $type['beginOffset'], - 'untilOffset' => $offset + strlen($tag), - 'members' => $type['members'], - ]; - } elseif ($stack !== [] && $stack[array_key_last($stack)]['union']) { - $stack[array_key_last($stack)]['members'][] = $content; + foreach ($unions as $union) { + if ($beginOffset >= $union['beginOffset'] && $untilOffset <= $union['untilOffset']) { + return true; } } - return $unions; + return false; } } diff --git a/src/Violation/Violation.php b/src/Violation/Violation.php index 4b1ef29..53c57a0 100644 --- a/src/Violation/Violation.php +++ b/src/Violation/Violation.php @@ -16,6 +16,7 @@ public function __construct( public string $message, public array $affectedRanges, public Severity $severity = Severity::WARNING, + public ?string $replacement = null, ) { if ($affectedRanges === []) { throw new \InvalidArgumentException('A violation must affect at least one source range.'); diff --git a/tests/Unit/Fix/MixedUnionFixerTest.php b/tests/Unit/Fix/MixedUnionFixerTest.php new file mode 100644 index 0000000..5c0f4e1 --- /dev/null +++ b/tests/Unit/Fix/MixedUnionFixerTest.php @@ -0,0 +1,64 @@ +mixedArray' + . 'example'; + + $file = new File('file.xml', $content); + $document = new \DOMDocument(); + $document->loadXML($content); + $violations = [ + ...new NativeTypeSniff(RunMode::Fix)->process($document, $file), + ...new MixedUnionSniff(RunMode::Fix)->process($document, $file), + ]; + self::assertCount(1, $violations); + self::assertSame('DocbookCS.MixedUnion', $violations[0]->sniffCode); + $fixer = new MixedUnionFixer(); + $result = new FixApplier()->apply($file, array_map($fixer->process(...), $violations)); + + self::assertSame( + 'mixedexample', + $result->file->content, + ); + self::assertSame(1, $result->applied); + } +} diff --git a/tests/Unit/Fix/NativeTypeFixerTest.php b/tests/Unit/Fix/NativeTypeFixerTest.php index 4ad2338..2a935cf 100644 --- a/tests/Unit/Fix/NativeTypeFixerTest.php +++ b/tests/Unit/Fix/NativeTypeFixerTest.php @@ -10,6 +10,7 @@ use DocbookCS\Fix\FixResult; use DocbookCS\Runner\RunMode; use DocbookCS\Sniff\NativeTypeSniff; +use DocbookCS\Sniff\MixedUnionDetector; use DocbookCS\Source\File; use DocbookCS\Source\Line; use DocbookCS\Violation\SourceRange; @@ -30,6 +31,7 @@ UsesClass(RunMode::class), UsesClass(SourceRange::class), UsesClass(Violation::class), + UsesClass(MixedUnionDetector::class), ] final class NativeTypeFixerTest extends TestCase { @@ -49,21 +51,6 @@ public function itCanonicalizesNativeTypeNames(): void self::assertSame(2, $result->applied); } - #[Test] - public function itCollapsesAUnionContainingMixed(): void - { - $content = 'mixednull' - . 'example'; - - $result = $this->fix($content); - - self::assertSame( - 'mixedexample', - $result->file->content, - ); - self::assertSame(1, $result->applied); - } - private function fix(string $content): FixResult { $file = new File('file.xml', $content); diff --git a/tests/Unit/Sniff/MixedUnionSniffTest.php b/tests/Unit/Sniff/MixedUnionSniffTest.php new file mode 100644 index 0000000..9d7cdb0 --- /dev/null +++ b/tests/Unit/Sniff/MixedUnionSniffTest.php @@ -0,0 +1,75 @@ +mixednull' + . 'example'; + + $violations = $this->process($content); + + self::assertCount(1, $violations); + self::assertSame('DocbookCS.MixedUnion', $violations[0]->sniffCode); + self::assertStringContainsString('union containing mixed', $violations[0]->message); + self::assertSame('mixed', $violations[0]->replacement); + self::assertSame( + 'mixednull', + $violations[0]->rangeOne()->content, + ); + } + + #[Test] + public function itAcceptsAUnionWithoutMixedAndMixedByItself(): void + { + $content = 'stringnull' + . 'examplemixed' + . 'value'; + + self::assertSame([], $this->process($content)); + } + + #[Test] + public function itDoesNotInspectUnionsOutsideSynopsesOrInsideComments(): void + { + $union = 'mixednull'; + $content = '' . $union . 'void' + . 'example'; + + self::assertSame([], $this->process($content)); + } + + /** @return list */ + private function process(string $content): array + { + $document = new \DOMDocument(); + $document->loadXML($content); + + return new MixedUnionSniff()->process($document, new File('file.xml', $content)); + } +} diff --git a/tests/Unit/Sniff/NativeTypeSniffTest.php b/tests/Unit/Sniff/NativeTypeSniffTest.php index 08ed0c2..ac4d18b 100644 --- a/tests/Unit/Sniff/NativeTypeSniffTest.php +++ b/tests/Unit/Sniff/NativeTypeSniffTest.php @@ -5,6 +5,7 @@ namespace DocbookCS\Tests\Unit\Sniff; use DocbookCS\Sniff\NativeTypeSniff; +use DocbookCS\Sniff\MixedUnionDetector; use DocbookCS\Source\File; use DocbookCS\Source\Line; use DocbookCS\Violation\SourceRange; @@ -20,6 +21,7 @@ UsesClass(File::class), UsesClass(Line::class), UsesClass(SourceRange::class), + UsesClass(MixedUnionDetector::class), ] final class NativeTypeSniffTest extends TestCase { @@ -46,22 +48,16 @@ public function itReportsNonCanonicalCasingAndAliases(): void self::assertSame('int', $this->replacementFromMessage($violations[1]->message)); self::assertSame('bool', $this->replacementFromMessage($violations[2]->message)); self::assertSame('Array', $violations[0]->rangeOne()->content); + self::assertSame('array', $violations[0]->replacement); } #[Test] - public function itReportsMixedCombinedWithAnotherUnionMember(): void + public function itDefersMembersOfRedundantMixedUnionsToTheMixedUnionSniff(): void { - $content = 'mixednull' + $content = 'mixedArray' . 'example'; - $violations = $this->process($content); - - self::assertCount(1, $violations); - self::assertStringContainsString('union containing mixed', $violations[0]->message); - self::assertSame( - 'mixednull', - $violations[0]->rangeOne()->content, - ); + self::assertSame([], $this->process($content)); } #[Test] From ec01e9ec886fcef0792f186c98100b29b3a7907b Mon Sep 17 00:00:00 2001 From: Lars Moelleken Date: Sun, 26 Jul 2026 01:56:20 +0200 Subject: [PATCH 3/5] Add PHPStan generics for fixer data --- src/Fix/Fixer/Fixer.php | 6 +++- src/Fix/Fixer/MixedUnionFixer.php | 10 ++++-- src/Fix/Fixer/NativeTypeFixer.php | 10 ++++-- src/Sniff/AbstractSniff.php | 10 ++++-- src/Sniff/Fixable.php | 6 +++- src/Sniff/MixedUnionSniff.php | 4 +++ src/Sniff/NativeTypeSniff.php | 4 +++ src/Sniff/SniffInterface.php | 3 +- src/Violation/Violation.php | 8 ++++- tests/Unit/Fix/MixedUnionFixerTest.php | 19 ++++++++++++ tests/Unit/Fix/NativeTypeFixerTest.php | 34 +++++++++++++++++++++ tests/Unit/Sniff/MixedUnionSniffTest.php | 2 +- tests/Unit/Sniff/NativeTypeSniffTest.php | 2 +- tests/Unit/Violation/AffectedRangesTest.php | 16 ++++++++++ 14 files changed, 120 insertions(+), 14 deletions(-) diff --git a/src/Fix/Fixer/Fixer.php b/src/Fix/Fixer/Fixer.php index a488c7f..8ddefdb 100644 --- a/src/Fix/Fixer/Fixer.php +++ b/src/Fix/Fixer/Fixer.php @@ -9,8 +9,12 @@ use DocbookCS\Fix\FixerException; use DocbookCS\Violation\Violation; +/** @template TFixerData = mixed */ interface Fixer { - /** @throws FixerException */ + /** + * @param Violation $violation + * @throws FixerException + */ public function process(Violation $violation): Fix|FixPlan; } diff --git a/src/Fix/Fixer/MixedUnionFixer.php b/src/Fix/Fixer/MixedUnionFixer.php index c289532..f809579 100644 --- a/src/Fix/Fixer/MixedUnionFixer.php +++ b/src/Fix/Fixer/MixedUnionFixer.php @@ -8,9 +8,13 @@ use DocbookCS\Fix\FixerException; use DocbookCS\Violation\Violation; +/** @implements Fixer */ final class MixedUnionFixer implements Fixer { - /** @throws FixerException */ + /** + * @param Violation $violation + * @throws FixerException + */ public function process(Violation $violation): Fix { $range = $violation->rangeOne(); @@ -19,12 +23,12 @@ public function process(Violation $violation): Fix } if ( - $violation->replacement !== 'mixed' + $violation->fixerData !== 'mixed' || preg_match('/^]*\bclass\s*=\s*(["\'])union\1[^>]*>.*<\/type>$/is', $range->content) !== 1 ) { throw FixerException::cannotFixInvalidContent($violation); } - return Fix::fromViolationAndRange($violation, $range, $violation->replacement); + return Fix::fromViolationAndRange($violation, $range, $violation->fixerData); } } diff --git a/src/Fix/Fixer/NativeTypeFixer.php b/src/Fix/Fixer/NativeTypeFixer.php index c891867..17067a9 100644 --- a/src/Fix/Fixer/NativeTypeFixer.php +++ b/src/Fix/Fixer/NativeTypeFixer.php @@ -8,9 +8,13 @@ use DocbookCS\Fix\FixerException; use DocbookCS\Violation\Violation; +/** @implements Fixer */ final class NativeTypeFixer implements Fixer { - /** @throws FixerException */ + /** + * @param Violation $violation + * @throws FixerException + */ public function process(Violation $violation): Fix { $range = $violation->rangeOne(); @@ -18,10 +22,10 @@ public function process(Violation $violation): Fix throw FixerException::cannotFixMissingContent(); } - if ($violation->replacement === null || preg_match('/^[a-z]+$/', $violation->replacement) !== 1) { + if (!is_string($violation->fixerData) || preg_match('/^[a-z]+$/', $violation->fixerData) !== 1) { throw FixerException::cannotFixInvalidContent($violation); } - return Fix::fromViolationAndRange($violation, $range, $violation->replacement); + return Fix::fromViolationAndRange($violation, $range, $violation->fixerData); } } diff --git a/src/Sniff/AbstractSniff.php b/src/Sniff/AbstractSniff.php index 9d88550..eaa4147 100644 --- a/src/Sniff/AbstractSniff.php +++ b/src/Sniff/AbstractSniff.php @@ -11,6 +11,10 @@ use DocbookCS\Violation\SourceRange; use DocbookCS\Violation\Violation; +/** + * @template TFixerData = mixed + * @implements SniffInterface + */ abstract class AbstractSniff implements SniffInterface { private const array NON_ELEMENT_DELIMITERS = [ @@ -84,14 +88,16 @@ protected function elementNameRanges(File $file, int $beginOffset, int $untilOff /** * @param non-empty-list $affectedRanges + * @param TFixerData $fixerData * + * @return Violation * @throws \InvalidArgumentException if the affected ranges are inconsistent */ protected function createViolation( string $filePath, string $message, array $affectedRanges, - ?string $replacement = null, + mixed $fixerData = null, ): Violation { return new Violation( sniffCode: static::getCode(), @@ -99,7 +105,7 @@ protected function createViolation( message: $message, affectedRanges: $affectedRanges, severity: $this->severity, - replacement: $replacement, + fixerData: $fixerData, ); } diff --git a/src/Sniff/Fixable.php b/src/Sniff/Fixable.php index 270bfab..d21ce0a 100644 --- a/src/Sniff/Fixable.php +++ b/src/Sniff/Fixable.php @@ -6,8 +6,12 @@ use DocbookCS\Fix\Fixer\Fixer; +/** + * @template TFixerData = mixed + * @extends SniffInterface + */ interface Fixable extends SniffInterface { - /** @return class-string */ + /** @return class-string> */ public static function getFixerClassName(): string; } diff --git a/src/Sniff/MixedUnionSniff.php b/src/Sniff/MixedUnionSniff.php index 4321aae..022c522 100644 --- a/src/Sniff/MixedUnionSniff.php +++ b/src/Sniff/MixedUnionSniff.php @@ -8,6 +8,10 @@ use DocbookCS\Source\File; use DocbookCS\Violation\SourceRange; +/** + * @extends AbstractSniff + * @implements Fixable + */ final class MixedUnionSniff extends AbstractSniff implements Fixable { public static function getCode(): string diff --git a/src/Sniff/NativeTypeSniff.php b/src/Sniff/NativeTypeSniff.php index e141caf..9f37c59 100644 --- a/src/Sniff/NativeTypeSniff.php +++ b/src/Sniff/NativeTypeSniff.php @@ -8,6 +8,10 @@ use DocbookCS\Source\File; use DocbookCS\Violation\SourceRange; +/** + * @extends AbstractSniff + * @implements Fixable + */ final class NativeTypeSniff extends AbstractSniff implements Fixable { private const array NATIVE_TYPES = [ diff --git a/src/Sniff/SniffInterface.php b/src/Sniff/SniffInterface.php index 17bc172..d733afe 100644 --- a/src/Sniff/SniffInterface.php +++ b/src/Sniff/SniffInterface.php @@ -12,6 +12,7 @@ * A sniff receives a DOMDocument (already loaded) and its source file, * then returns zero or more findings for reports and optional fixes. */ +/** @template TFixerData = mixed */ interface SniffInterface { public RunMode $mode { get; } @@ -26,7 +27,7 @@ public static function getCode(): string; /** * Apply the sniff to the given document. * - * @return list + * @return list> */ public function process(\DOMDocument $document, File $file): array; diff --git a/src/Violation/Violation.php b/src/Violation/Violation.php index 53c57a0..a26903d 100644 --- a/src/Violation/Violation.php +++ b/src/Violation/Violation.php @@ -4,6 +4,7 @@ namespace DocbookCS\Violation; +/** @template TFixerData = mixed */ final readonly class Violation { /** @@ -16,7 +17,12 @@ public function __construct( public string $message, public array $affectedRanges, public Severity $severity = Severity::WARNING, - public ?string $replacement = null, + /** + * Data passed from the sniff to its fixer. Fixers are responsible for validating its shape. + * + * @var TFixerData + */ + public mixed $fixerData = null, ) { if ($affectedRanges === []) { throw new \InvalidArgumentException('A violation must affect at least one source range.'); diff --git a/tests/Unit/Fix/MixedUnionFixerTest.php b/tests/Unit/Fix/MixedUnionFixerTest.php index 5c0f4e1..216fc00 100644 --- a/tests/Unit/Fix/MixedUnionFixerTest.php +++ b/tests/Unit/Fix/MixedUnionFixerTest.php @@ -7,6 +7,7 @@ use DocbookCS\Fix\Fix; use DocbookCS\Fix\FixApplier; use DocbookCS\Fix\Fixer\MixedUnionFixer; +use DocbookCS\Fix\FixerException; use DocbookCS\Fix\FixResult; use DocbookCS\Runner\RunMode; use DocbookCS\Sniff\MixedUnionDetector; @@ -61,4 +62,22 @@ public function itCollapsesAUnionContainingMixed(): void ); self::assertSame(1, $result->applied); } + + #[Test] + public function itRejectsUnexpectedFixerData(): void + { + $content = 'mixedint'; + $violation = new Violation( + 'DocbookCS.MixedUnion', + 'file.xml', + 'Message', + [new SourceRange(1, 0, strlen($content), $content)], + fixerData: ['replacement' => 'mixed'], + ); + + $this->expectException(FixerException::class); + + // Bypass the PHPStan generic contract to exercise the runtime boundary. + new \ReflectionMethod(MixedUnionFixer::class, 'process')->invoke(new MixedUnionFixer(), $violation); + } } diff --git a/tests/Unit/Fix/NativeTypeFixerTest.php b/tests/Unit/Fix/NativeTypeFixerTest.php index 2a935cf..69cab11 100644 --- a/tests/Unit/Fix/NativeTypeFixerTest.php +++ b/tests/Unit/Fix/NativeTypeFixerTest.php @@ -7,6 +7,7 @@ use DocbookCS\Fix\Fix; use DocbookCS\Fix\FixApplier; use DocbookCS\Fix\Fixer\NativeTypeFixer; +use DocbookCS\Fix\FixerException; use DocbookCS\Fix\FixResult; use DocbookCS\Runner\RunMode; use DocbookCS\Sniff\NativeTypeSniff; @@ -51,6 +52,39 @@ public function itCanonicalizesNativeTypeNames(): void self::assertSame(2, $result->applied); } + #[Test] + public function itRejectsNonStringFixerData(): void + { + $violation = new Violation( + 'DocbookCS.NativeType', + 'file.xml', + 'Message', + [new SourceRange(1, 0, 5, 'Array')], + fixerData: ['replacement' => 'array'], + ); + + $this->expectException(FixerException::class); + + // Bypass the PHPStan generic contract to exercise the runtime boundary. + new \ReflectionMethod(NativeTypeFixer::class, 'process')->invoke(new NativeTypeFixer(), $violation); + } + + #[Test] + public function itRejectsAnInvalidReplacement(): void + { + $violation = new Violation( + 'DocbookCS.NativeType', + 'file.xml', + 'Message', + [new SourceRange(1, 0, 5, 'Array')], + fixerData: '', + ); + + $this->expectException(FixerException::class); + + new NativeTypeFixer()->process($violation); + } + private function fix(string $content): FixResult { $file = new File('file.xml', $content); diff --git a/tests/Unit/Sniff/MixedUnionSniffTest.php b/tests/Unit/Sniff/MixedUnionSniffTest.php index 9d7cdb0..f665a3c 100644 --- a/tests/Unit/Sniff/MixedUnionSniffTest.php +++ b/tests/Unit/Sniff/MixedUnionSniffTest.php @@ -36,7 +36,7 @@ public function itReportsMixedCombinedWithAnotherUnionMember(): void self::assertCount(1, $violations); self::assertSame('DocbookCS.MixedUnion', $violations[0]->sniffCode); self::assertStringContainsString('union containing mixed', $violations[0]->message); - self::assertSame('mixed', $violations[0]->replacement); + self::assertSame('mixed', $violations[0]->fixerData); self::assertSame( 'mixednull', $violations[0]->rangeOne()->content, diff --git a/tests/Unit/Sniff/NativeTypeSniffTest.php b/tests/Unit/Sniff/NativeTypeSniffTest.php index ac4d18b..3b9bf78 100644 --- a/tests/Unit/Sniff/NativeTypeSniffTest.php +++ b/tests/Unit/Sniff/NativeTypeSniffTest.php @@ -48,7 +48,7 @@ public function itReportsNonCanonicalCasingAndAliases(): void self::assertSame('int', $this->replacementFromMessage($violations[1]->message)); self::assertSame('bool', $this->replacementFromMessage($violations[2]->message)); self::assertSame('Array', $violations[0]->rangeOne()->content); - self::assertSame('array', $violations[0]->replacement); + self::assertSame('array', $violations[0]->fixerData); } #[Test] diff --git a/tests/Unit/Violation/AffectedRangesTest.php b/tests/Unit/Violation/AffectedRangesTest.php index d9c050d..e44b84c 100644 --- a/tests/Unit/Violation/AffectedRangesTest.php +++ b/tests/Unit/Violation/AffectedRangesTest.php @@ -57,6 +57,22 @@ public function itRejectsUnorderedOrOverlappingAffectedRanges(): void ]); } + #[Test] + public function itTransportsArbitraryFixerData(): void + { + $fixerData = ['replacement' => 'int', 'attributes' => ['role' => 'return']]; + + $violation = new Violation( + 'Test', + 'file.xml', + 'Message', + [new SourceRange(1, 0, 1)], + fixerData: $fixerData, + ); + + self::assertSame($fixerData, $violation->fixerData); + } + #[Test, DataProvider('invalidSourceRanges')] public function itRejectsInvalidSourceRanges(int $line, int $beginOffset, int $untilOffset, ?string $content): void { From b84b9805ae31a5ea103303e6892442b155ba11b7 Mon Sep 17 00:00:00 2001 From: Lars Moelleken Date: Sun, 26 Jul 2026 14:33:33 +0200 Subject: [PATCH 4/5] Fix generic test stubs and coverage metadata --- tests/Unit/Fix/MixedUnionFixerTest.php | 3 +++ tests/Unit/Fix/NativeTypeFixerTest.php | 3 +++ tests/Unit/Runner/SniffRunnerTest.php | 6 +++--- tests/Unit/Runner/XmlFileProcessorTest.php | 6 +++--- 4 files changed, 12 insertions(+), 6 deletions(-) diff --git a/tests/Unit/Fix/MixedUnionFixerTest.php b/tests/Unit/Fix/MixedUnionFixerTest.php index 216fc00..1dde6e9 100644 --- a/tests/Unit/Fix/MixedUnionFixerTest.php +++ b/tests/Unit/Fix/MixedUnionFixerTest.php @@ -6,6 +6,7 @@ use DocbookCS\Fix\Fix; use DocbookCS\Fix\FixApplier; +use DocbookCS\Fix\FixPlan; use DocbookCS\Fix\Fixer\MixedUnionFixer; use DocbookCS\Fix\FixerException; use DocbookCS\Fix\FixResult; @@ -28,6 +29,8 @@ UsesClass(File::class), UsesClass(Fix::class), UsesClass(FixApplier::class), + UsesClass(FixPlan::class), + UsesClass(FixerException::class), UsesClass(FixResult::class), UsesClass(Line::class), UsesClass(RunMode::class), diff --git a/tests/Unit/Fix/NativeTypeFixerTest.php b/tests/Unit/Fix/NativeTypeFixerTest.php index 69cab11..0d066b7 100644 --- a/tests/Unit/Fix/NativeTypeFixerTest.php +++ b/tests/Unit/Fix/NativeTypeFixerTest.php @@ -6,6 +6,7 @@ use DocbookCS\Fix\Fix; use DocbookCS\Fix\FixApplier; +use DocbookCS\Fix\FixPlan; use DocbookCS\Fix\Fixer\NativeTypeFixer; use DocbookCS\Fix\FixerException; use DocbookCS\Fix\FixResult; @@ -27,6 +28,8 @@ UsesClass(File::class), UsesClass(Fix::class), UsesClass(FixApplier::class), + UsesClass(FixPlan::class), + UsesClass(FixerException::class), UsesClass(FixResult::class), UsesClass(Line::class), UsesClass(RunMode::class), diff --git a/tests/Unit/Runner/SniffRunnerTest.php b/tests/Unit/Runner/SniffRunnerTest.php index 44657f3..292ec07 100644 --- a/tests/Unit/Runner/SniffRunnerTest.php +++ b/tests/Unit/Runner/SniffRunnerTest.php @@ -143,7 +143,7 @@ public function itCallsProgressMethods(): void #[Test] // TODO: should be integration public function itAddsFileReportsForFilesWithViolations(): void { - $sniff = new class (RunMode::Sniff) implements SniffInterface { + $sniff = new /** @implements SniffInterface */ class (RunMode::Sniff) implements SniffInterface { public function __construct(public RunMode $mode) { } @@ -184,7 +184,7 @@ public function setProperty(string $name, string $value): void #[Test] // TODO: should be integration public function itStoresAbsolutePathsInFileReports(): void { - $sniff = new class (RunMode::Sniff) implements SniffInterface { + $sniff = new /** @implements SniffInterface */ class (RunMode::Sniff) implements SniffInterface { public function __construct(public RunMode $mode) { } @@ -385,7 +385,7 @@ public function itScansLexicallyEquivalentWideTargetsOnlyOnce(): void #[Test] // TODO: should be integration public function itReportsNoViolationsForFilesInDiffWithoutAddedLines(): void { - $sniff = new class (RunMode::Sniff) implements SniffInterface { + $sniff = new /** @implements SniffInterface */ class (RunMode::Sniff) implements SniffInterface { public function __construct(public RunMode $mode) { } diff --git a/tests/Unit/Runner/XmlFileProcessorTest.php b/tests/Unit/Runner/XmlFileProcessorTest.php index 28f4b55..be39512 100644 --- a/tests/Unit/Runner/XmlFileProcessorTest.php +++ b/tests/Unit/Runner/XmlFileProcessorTest.php @@ -307,7 +307,7 @@ public function itReportsNoViolationsInDiffModeWhenNoLinesWereAdded(): void #[Test] public function itDoesNotFixViolationsFromNonFixableSniffs(): void { - $sniff = new class (RunMode::Sniff) implements SniffInterface { + $sniff = new /** @implements SniffInterface */ class (RunMode::Sniff) implements SniffInterface { public function __construct(public RunMode $mode) { } @@ -343,7 +343,7 @@ public function setProperty(string $name, string $value): void #[Test] public function itThrowsWhenFixableSniffReportsViolationWithoutContentInFixMode(): void { - $sniff = new class (RunMode::Fix) implements Fixable { + $sniff = new /** @implements Fixable */ class (RunMode::Fix) implements Fixable { public function __construct(public RunMode $mode) { } @@ -385,7 +385,7 @@ public function setProperty(string $name, string $value): void /** @param list $lines */ private function sniff(array $lines): SniffInterface { - $sniff = new class (RunMode::Sniff) implements SniffInterface { + $sniff = new /** @implements SniffInterface */ class (RunMode::Sniff) implements SniffInterface { /** @var list */ public array $lines = []; From 981cf199bf6093c5cdbd96d7f9690bf5bf9a996c Mon Sep 17 00:00:00 2001 From: Lars Moelleken Date: Sun, 26 Jul 2026 15:15:48 +0200 Subject: [PATCH 5/5] Merge upstream main and resolve test conflicts --- src/Application.php | 4 +- src/Config/ConfigParser.php | 41 +-- src/Diff/UpstreamResolver.php | 3 +- src/Fix/FixerException.php | 5 - src/Path/EntityResolver.php | 6 +- src/Process/NativeProcessRunner.php | 2 +- src/Progress/ConsoleProgress.php | 20 +- src/RelativePath.php | 6 +- src/Report/FileReport.php | 227 +++++++++++-- src/Report/Report.php | 205 ++++++++--- src/Report/ReportException.php | 18 + src/Report/Reporter/CheckstyleReporter.php | 8 +- src/Report/Reporter/ConsoleReporter.php | 176 +++++++--- src/Report/Reporter/JsonReporter.php | 22 +- src/Runner/RunCoordinator.php | 70 ++-- src/Runner/RunScopeResolver.php | 7 - src/Runner/XmlFileProcessor.php | 137 ++------ src/Runner/XmlFixRunner.php | 63 ++++ src/Runner/XmlProcessingResult.php | 34 -- src/Runner/XmlSniffRunner.php | 101 ++++++ src/Sniff/AbstractSniff.php | 97 ------ src/Sniff/AttributeOrderSniff.php | 2 +- src/Sniff/ExceptionNameSniff.php | 4 +- src/Sniff/MixedUnionSniff.php | 2 +- src/Sniff/NativeTypeSniff.php | 2 +- src/Sniff/SimparaSniff.php | 20 +- src/Sniff/SniffInterface.php | 5 - src/Source/File.php | 97 ++++++ src/Xml/XmlParser.php | 67 ++++ tests/Support/XmlHelper.php | 16 + tests/Unit/ApplicationInputTest.php | 90 +++++ tests/Unit/ApplicationTest.php | 48 +-- tests/Unit/Config/ConfigParserTest.php | 4 + tests/Unit/Diff/DiffChangesetTest.php | 37 ++ tests/Unit/Diff/UpstreamResolverTest.php | 116 +++++++ tests/Unit/Fix/AttributeOrderFixerTest.php | 9 +- tests/Unit/Fix/ExceptionNameFixerTest.php | 13 +- tests/Unit/Fix/FixPlanTest.php | 67 ++++ tests/Unit/Fix/FixerInputValidationTest.php | 130 +++++++ tests/Unit/Fix/MixedUnionFixerTest.php | 3 + tests/Unit/Fix/NativeTypeFixerTest.php | 3 + tests/Unit/Fix/SimparaFixerTest.php | 13 +- .../Unit/Fix/WhitespaceConcernFixersTest.php | 12 +- .../Unit/Process/NativeProcessRunnerTest.php | 14 + tests/Unit/Progress/ConsoleProgressTest.php | 14 +- tests/Unit/Report/ReportTest.php | 317 ++++++++++++++---- .../Reporter/CheckstyleReporterTest.php | 46 ++- .../Report/Reporter/ConsoleReporterTest.php | 272 +++++++++++---- .../Unit/Report/Reporter/JsonReporterTest.php | 98 ++++-- tests/Unit/Runner/FixConvergenceTest.php | 72 ++-- .../Runner/RunCoordinatorFileFailureTest.php | 121 ++++++- tests/Unit/Runner/RunReportingTest.php | 113 +++++++ tests/Unit/Runner/RunScopeResolverTest.php | 25 ++ tests/Unit/Runner/RunScopeTest.php | 47 ++- tests/Unit/Runner/SniffRunnerTest.php | 135 +++----- tests/Unit/Runner/SourceRangeScopeTest.php | 51 +-- tests/Unit/Runner/SourceScopeTest.php | 48 ++- .../Runner/XmlFileProcessorPipelineTest.php | 262 +++++++++------ tests/Unit/Runner/XmlProcessingResultTest.php | 73 ---- ...ocessorTest.php => XmlSniffRunnerTest.php} | 141 ++++---- tests/Unit/Sniff/ExceptionNameSniffTest.php | 14 + tests/Unit/Sniff/SimparaSniffTest.php | 11 + .../Sniff/WhitespaceConcernSniffsTest.php | 16 + tests/Unit/Source/FileTest.php | 32 ++ tests/Unit/Xml/XmlParserTest.php | 57 ++++ 65 files changed, 2881 insertions(+), 1110 deletions(-) create mode 100644 src/Report/ReportException.php create mode 100644 src/Runner/XmlFixRunner.php delete mode 100644 src/Runner/XmlProcessingResult.php create mode 100644 src/Runner/XmlSniffRunner.php create mode 100644 src/Xml/XmlParser.php create mode 100644 tests/Support/XmlHelper.php create mode 100644 tests/Unit/Diff/DiffChangesetTest.php create mode 100644 tests/Unit/Diff/UpstreamResolverTest.php create mode 100644 tests/Unit/Fix/FixerInputValidationTest.php create mode 100644 tests/Unit/Runner/RunReportingTest.php delete mode 100644 tests/Unit/Runner/XmlProcessingResultTest.php rename tests/Unit/Runner/{XmlFileProcessorTest.php => XmlSniffRunnerTest.php} (74%) create mode 100644 tests/Unit/Xml/XmlParserTest.php diff --git a/src/Application.php b/src/Application.php index 64f2fa9..934d1f6 100644 --- a/src/Application.php +++ b/src/Application.php @@ -119,7 +119,7 @@ public function run(): int $progress = $this->createProgress($options); try { - $report = new RunCoordinator($progress)->run($runPlan); + $report = new RunCoordinator($progress, collectPerformance: $options['perf'])->runWithMetrics($runPlan); } catch (\Throwable $e) { $this->writeError('Runtime error: ' . $e->getMessage() . PHP_EOL); @@ -134,7 +134,7 @@ public function run(): int $this->write($reporter->generate($report)); - return (int) $report->hasViolations(); + return (int) $report->hasFinalViolations(); } /** diff --git a/src/Config/ConfigParser.php b/src/Config/ConfigParser.php index 9a091b8..9d01f97 100644 --- a/src/Config/ConfigParser.php +++ b/src/Config/ConfigParser.php @@ -4,10 +4,16 @@ namespace DocbookCS\Config; +use DocbookCS\Xml\XmlParser; + final class ConfigParser { private const string NAMESPACE_URI = 'https://php.github.io/docbook-cs/config'; + public function __construct(private readonly XmlParser $xmlParser = new XmlParser()) + { + } + /** * @throws ConfigParserException if the file cannot be read or contains invalid XML. * @throws \InvalidArgumentException if a SniffEntry is constructed with an invalid class name. @@ -19,21 +25,13 @@ public function parseFile(string $filePath): ConfigData } $basePath = dirname(realpath($filePath) ?: ''); + $xml = $this->xmlParser->parseElement(file_get_contents($filePath) ?: ''); - $previousUseErrors = libxml_use_internal_errors(true); - $xml = simplexml_load_string(file_get_contents($filePath) ?: ''); - $errors = libxml_get_errors(); - libxml_clear_errors(); - libxml_use_internal_errors($previousUseErrors); - - if ($xml === false) { - $message = $errors !== [] - ? $errors[0]->message - : 'Unknown parse error'; // @codeCoverageIgnore - throw ConfigParserException::invalidXml($filePath, trim($message)); + if ($xml instanceof \LibXMLError) { + throw ConfigParserException::invalidXml($filePath, trim($xml->message)); } - return $this->parse($xml, $basePath); + return $this->createConfigData($xml, $basePath); } /** @@ -42,27 +40,20 @@ public function parseFile(string $filePath): ConfigData */ public function parseString(string $xmlContent, string $basePath): ConfigData { - $previousUseErrors = libxml_use_internal_errors(true); - $xml = simplexml_load_string($xmlContent); - $errors = libxml_get_errors(); - libxml_clear_errors(); - libxml_use_internal_errors($previousUseErrors); - - if ($xml === false) { - $message = $errors !== [] - ? $errors[0]->message - : 'Unknown parse error'; // @codeCoverageIgnore - throw ConfigParserException::invalidXml('(string)', trim($message)); + $xml = $this->xmlParser->parseElement($xmlContent); + + if ($xml instanceof \LibXMLError) { + throw ConfigParserException::invalidXml('(string)', trim($xml->message)); } - return $this->parse($xml, $basePath); + return $this->createConfigData($xml, $basePath); } /** * @throws ConfigParserException if the XML is invalid or required elements/attributes are missing. * @throws \InvalidArgumentException if a SniffEntry is constructed with an invalid class name. */ - private function parse(\SimpleXMLElement $root, string $basePath): ConfigData + private function createConfigData(\SimpleXMLElement $root, string $basePath): ConfigData { // Register the namespace for xpath queries. $root->registerXPathNamespace('d', self::NAMESPACE_URI); diff --git a/src/Diff/UpstreamResolver.php b/src/Diff/UpstreamResolver.php index 4fc45ae..6705b7c 100644 --- a/src/Diff/UpstreamResolver.php +++ b/src/Diff/UpstreamResolver.php @@ -48,8 +48,9 @@ public function resolve(string $repoRoot, string $repoName): ?string } try { + // filesystem or OS edge cases if (!flock($lock, LOCK_EX)) { - return null; + return null; // @codeCoverageIgnore } return $this->refreshAndResolve($repoRoot, $repoName); diff --git a/src/Fix/FixerException.php b/src/Fix/FixerException.php index 7fe9815..998cf34 100644 --- a/src/Fix/FixerException.php +++ b/src/Fix/FixerException.php @@ -28,11 +28,6 @@ public static function cannotFixInvalidContent(Violation $violation): self )); } - public static function cannotReadFixedContent(): self - { - return new self('Cannot read fixed content when no fix application was attempted.'); - } - public static function invalidFixedXml(string $filePath): self { return new self( diff --git a/src/Path/EntityResolver.php b/src/Path/EntityResolver.php index aafb27b..a4fd274 100644 --- a/src/Path/EntityResolver.php +++ b/src/Path/EntityResolver.php @@ -123,16 +123,16 @@ private function scanDirectory(string $directory): array */ private function resolveFile(string $filePath, array &$visited, array &$paths, ?string $originEntity = null): array { - if (isset($visited[$filePath]) || !is_readable($filePath)) { + if (isset($visited[$filePath])) { return []; } $visited[$filePath] = true; - $content = file_get_contents($filePath); + $content = @file_get_contents($filePath); if ($content === false) { - return []; // @codeCoverageIgnore + return []; } $entities = $this->extractEntities($content, $filePath, $visited, $paths); diff --git a/src/Process/NativeProcessRunner.php b/src/Process/NativeProcessRunner.php index 8e8907c..8c12354 100644 --- a/src/Process/NativeProcessRunner.php +++ b/src/Process/NativeProcessRunner.php @@ -8,7 +8,7 @@ final class NativeProcessRunner implements ProcessRunnerInterface { public function run(array $command, string $workingDirectory, array $environment = []): ProcessResult { - $process = proc_open( + $process = @proc_open( $command, [ ['pipe', 'r'], diff --git a/src/Progress/ConsoleProgress.php b/src/Progress/ConsoleProgress.php index 899235a..5b6c32b 100644 --- a/src/Progress/ConsoleProgress.php +++ b/src/Progress/ConsoleProgress.php @@ -37,7 +37,12 @@ public function start(int $totalFiles): void return; } - $this->write($this->dim(sprintf('Scanning %d file(s)...', $totalFiles)) . PHP_EOL); + $suffix = $totalFiles === 1 ? 'file' : 'files'; + $formattedTotal = number_format($totalFiles); + + $message = sprintf('Scanning %s %s...', $formattedTotal, $suffix); + + $this->write($this->dim($message) . PHP_EOL); $this->drawBar(0, ''); } @@ -62,13 +67,14 @@ public function finish(): void private function drawBar(int $current, string $filePath, int $violations = 0): void { - $percent = $this->totalFiles > 0 - ? (int)floor(($current / $this->totalFiles) * 100) - : 0; // @codeCoverageIgnore + if ($this->totalFiles === 0) { + // prevented by public methods + return; // @codeCoverageIgnore + } - $filled = $this->totalFiles > 0 - ? (int)floor(($current / $this->totalFiles) * self::BAR_WIDTH) - : 0; // @codeCoverageIgnore + $ratio = $current / $this->totalFiles; + $percent = (int)floor($ratio * 100); + $filled = (int)floor($ratio * self::BAR_WIDTH); $empty = self::BAR_WIDTH - $filled; diff --git a/src/RelativePath.php b/src/RelativePath.php index 1f0689b..3a7dac5 100644 --- a/src/RelativePath.php +++ b/src/RelativePath.php @@ -8,11 +8,7 @@ final class RelativePath { public static function fromWorkingDirectory(string $filePath): string { - $workingDirectory = getcwd(); - if ($workingDirectory === false) { - return $filePath; // @codeCoverageIgnore - } - + $workingDirectory = getcwd() ?: '.'; $prefix = rtrim(str_replace('\\', '/', $workingDirectory), '/') . '/'; $normalisedPath = str_replace('\\', '/', $filePath); diff --git a/src/Report/FileReport.php b/src/Report/FileReport.php index 2ec645c..02ec303 100644 --- a/src/Report/FileReport.php +++ b/src/Report/FileReport.php @@ -9,56 +9,243 @@ final class FileReport { + public private(set) float $totalSniffingTime = 0.0; + + public private(set) float $totalFixingTime = 0.0; + + /** @var array */ + public private(set) array $sniffingTimes = []; + + /** @var array */ + public private(set) array $fixingTimes = []; + + public private(set) int $fixingPasses = 0; + + public private(set) bool $changed = false; + /** @var list */ - public private(set) array $violations = []; + public private(set) array $foundViolations; + + /** @var list */ + public private(set) array $finalViolations; public function __construct( public readonly string $filePath, + private readonly bool $collectPerformance = false, ) { } - public function addViolation(Violation $violation): void + public function markChanged(): void { - $this->violations[] = $violation; + $this->changed = true; } - /** @param list $violations */ - public function addViolations(array $violations): void + /** @throws ReportException if found violations were already added */ + public function addFailedViolation(Violation $violation): void + { + if (isset($this->foundViolations)) { + throw ReportException::foundViolationsAlreadyAdded($this->filePath); + } + + $this->foundViolations = [$violation]; + $this->finalViolations = [$violation]; + } + + /** + * @param list $violations + * @throws ReportException if found violations were already added + */ + public function addFoundViolations(array $violations): void + { + if (isset($this->foundViolations)) { + throw ReportException::foundViolationsAlreadyAdded($this->filePath); + } + + $this->foundViolations = $violations; + $this->finalViolations = $violations; + } + + public function getFoundViolationCount(): int + { + return isset($this->foundViolations) ? count($this->foundViolations) : 0; + } + + /** + * @param list $violations + * @throws ReportException if found violations were not added + */ + public function addFinalViolations(array $violations): void { - foreach ($violations as $violation) { - $this->addViolation($violation); + if (!isset($this->foundViolations)) { + throw ReportException::cannotSetFinalViolationsBeforeFoundViolations($this->filePath); } + + $this->finalViolations = $violations; + } + + public function hasFinalViolations(): bool + { + return isset($this->finalViolations) && $this->finalViolations !== []; + } + + public function getFinalViolationCount(): int + { + return isset($this->finalViolations) ? count($this->finalViolations) : 0; + } + + public function getAppliedFixesCount(): int + { + return $this->fixingPasses > 0 + ? max(0, $this->getFoundViolationCount() - $this->getFinalViolationCount()) + : 0; } - /** @return list */ - public function getViolations(): array + public function getSkippedFixesCount(): int { - return $this->violations; + return $this->fixingPasses > 0 + ? $this->getFinalViolationCount() + : 0; } - public function getViolationCount(): int + public function getFixedErrorCount(): int { - return count($this->violations); + return $this->getFixedSeverityCount(Severity::ERROR); } - public function hasViolations(): bool + public function getFixedWarningCount(): int { - return $this->violations !== []; + return $this->getFixedSeverityCount(Severity::WARNING); + } + + public function recordFixingPass(): void + { + $this->fixingPasses++; } public function getErrorCount(): int { - return array_filter( - $this->violations, - static fn(Violation $v): bool => $v->severity === Severity::ERROR, - ) |> count(...); + return $this->countSeverity(Severity::ERROR); } public function getWarningCount(): int + { + return $this->countSeverity(Severity::WARNING); + } + + public function getInfoCount(): int + { + return $this->countSeverity(Severity::INFO); + } + + /** + * @template T + * @param callable(): T $operation + * @return T + */ + public function measureFixing(callable $operation): mixed + { + if (!$this->collectPerformance) { + return $operation(); + } + + $start = microtime(true); + + try { + return $operation(); + } finally { + $this->totalFixingTime += microtime(true) - $start; + } + } + + /** + * @template T + * @param callable(): T $operation + * @return T + */ + public function measureFixer(string $sniffCode, callable $operation): mixed + { + if (!$this->collectPerformance) { + return $operation(); + } + + $start = microtime(true); + + try { + return $operation(); + } finally { + $this->fixingTimes[$sniffCode] ??= 0.0; + $this->fixingTimes[$sniffCode] += microtime(true) - $start; + } + } + + /** + * @template T + * @param callable(): T $operation + * @return T + */ + public function measureSniffing(callable $operation): mixed + { + if (!$this->collectPerformance) { + return $operation(); + } + + $start = microtime(true); + + try { + return $operation(); + } finally { + $this->totalSniffingTime += microtime(true) - $start; + } + } + + /** + * @template T + * @param callable(): T $operation + * @return T + */ + public function measureSniffer(string $sniffCode, callable $operation): mixed + { + if (!$this->collectPerformance) { + return $operation(); + } + + $start = microtime(true); + + try { + return $operation(); + } finally { + $this->sniffingTimes[$sniffCode] ??= 0.0; + $this->sniffingTimes[$sniffCode] += microtime(true) - $start; + } + } + + private function countSeverity(Severity $severity): int + { + if (!isset($this->finalViolations)) { + return 0; + } + + return $this->countViolationSeverity($this->finalViolations, $severity); + } + + private function getFixedSeverityCount(Severity $severity): int + { + if ($this->fixingPasses === 0 || !isset($this->foundViolations, $this->finalViolations)) { + return 0; + } + + $foundViolations = $this->countViolationSeverity($this->foundViolations, $severity); + $finalViolations = $this->countViolationSeverity($this->finalViolations, $severity); + + return max(0, $foundViolations - $finalViolations); + } + + /** @param list $violations */ + private function countViolationSeverity(array $violations, Severity $severity): int { return array_filter( - $this->violations, - static fn(Violation $v): bool => $v->severity === Severity::WARNING, + $violations, + static fn(Violation $violation): bool => $violation->severity === $severity, ) |> count(...); } } diff --git a/src/Report/Report.php b/src/Report/Report.php index 77e7335..e993874 100644 --- a/src/Report/Report.php +++ b/src/Report/Report.php @@ -8,107 +8,212 @@ final class Report { + public private(set) float $totalTime = 0.0; + /** @var array */ - private array $fileReports = []; + public private(set) array $fileReports = []; - private int $filesScanned = 0; + public function __construct(private readonly bool $collectPerformance = false) + { + } - private float $totalTime = 0.0; + /** + * @template T + * @param callable(): T $operation + * @return T + */ + public function measureWallTime(callable $operation): mixed + { + $start = microtime(true); - /** @var array */ - private array $sniffTimes = []; + try { + return $operation(); + } finally { + $this->totalTime = microtime(true) - $start; + } + } + + public function newFileReport(string $filePath): FileReport + { + return $this->fileReports[$filePath] = new FileReport($filePath, $this->collectPerformance); + } public function addFileReport(FileReport $fileReport): void { $this->fileReports[$fileReport->filePath] = $fileReport; } - public function incrementFilesScanned(): void + public function getScannedFilesCount(): int { - $this->filesScanned++; + return count($this->fileReports); } - public function getFilesScanned(): int + public function getViolatingFilesCount(): int { - return $this->filesScanned; + return array_filter( + $this->fileReports, + static fn(FileReport $fileReport): bool => $fileReport->hasFinalViolations(), + ) |> count(...); } - /** @return array */ - public function getFileReports(): array + public function getChangedFilesCount(): int { - return $this->fileReports; + return array_filter( + $this->fileReports, + static fn(FileReport $fileReport): bool => $fileReport->changed, + ) |> count(...); } - public function getTotalViolations(): int + public function getFoundViolationsCount(): int { - $total = 0; - foreach ($this->fileReports as $fr) { - $total += $fr->getViolationCount(); - } + return array_sum(array_map( + static fn(FileReport $fileReport): int => $fileReport->getFoundViolationCount(), + $this->fileReports, + )); + } - return $total; + public function getAppliedFixesCount(): int + { + return array_sum(array_map( + static fn(FileReport $fileReport): int => $fileReport->getAppliedFixesCount(), + $this->fileReports, + )); } - public function getTotalErrors(): int + public function getSkippedFixesCount(): int { - $total = 0; - foreach ($this->fileReports as $fr) { - $total += $fr->getErrorCount(); - } + return array_sum(array_map( + static fn(FileReport $fileReport): int => $fileReport->getSkippedFixesCount(), + $this->fileReports, + )); + } - return $total; + public function getFixedErrorCount(): int + { + return array_sum(array_map( + static fn(FileReport $fileReport): int => $fileReport->getFixedErrorCount(), + $this->fileReports, + )); } - public function getTotalWarnings(): int + public function getFixedWarningCount(): int { - $total = 0; - foreach ($this->fileReports as $fr) { - $total += $fr->getWarningCount(); - } + return array_sum(array_map( + static fn(FileReport $fileReport): int => $fileReport->getFixedWarningCount(), + $this->fileReports, + )); + } - return $total; + public function hasFixingResults(): bool + { + return array_any($this->fileReports, static fn(FileReport $fileReport): bool => $fileReport->fixingPasses > 0); } - public function hasViolations(): bool + public function getFixingPassesCount(): int { - return $this->getTotalViolations() > 0; + return array_sum(array_map( + static fn(FileReport $fileReport): int => $fileReport->fixingPasses, + $this->fileReports, + )); } - /** @return list */ - public function getAllViolations(): array + public function getTotalFixingTime(): float + { + return array_sum(array_map( + static fn(FileReport $fileReport): float => $fileReport->totalFixingTime, + $this->fileReports, + )); + } + + public function getTotalSniffingTime(): float + { + return array_sum(array_map( + static fn(FileReport $fileReport): float => $fileReport->totalSniffingTime, + $this->fileReports, + )); + } + + /** @return array */ + public function getFixingTimes(): array { - $all = []; + $fixingTimes = []; + foreach ($this->fileReports as $fileReport) { - foreach ($fileReport->getViolations() as $violation) { - $all[] = $violation; + foreach ($fileReport->fixingTimes as $sniffCode => $time) { + $fixingTimes[$sniffCode] ??= 0.0; + $fixingTimes[$sniffCode] += $time; } } - return $all; + return $fixingTimes; } - public function setTotalTime(float $time): void + /** @return array */ + public function getSniffingTimes(): array { - $this->totalTime = $time; + $sniffingTimes = []; + + foreach ($this->fileReports as $fileReport) { + foreach ($fileReport->sniffingTimes as $sniffCode => $time) { + $sniffingTimes[$sniffCode] ??= 0.0; + $sniffingTimes[$sniffCode] += $time; + } + } + + return $sniffingTimes; } - public function addSniffTime(string $sniffClass, float $time): void + public function getTotalFinalViolationCount(): int { - if (!isset($this->sniffTimes[$sniffClass])) { - $this->sniffTimes[$sniffClass] = 0.0; - } + return array_sum(array_map( + static fn(FileReport $fileReport): int => $fileReport->getFinalViolationCount(), + $this->fileReports, + )); + } - $this->sniffTimes[$sniffClass] += $time; + public function getTotalErrorLevelViolationCount(): int + { + return array_sum(array_map( + static fn(FileReport $fileReport): int => $fileReport->getErrorCount(), + $this->fileReports, + )); } - public function getTotalTime(): float + public function getTotalWarningLevelViolationCount(): int { - return $this->totalTime; + return array_sum(array_map( + static fn(FileReport $fileReport): int => $fileReport->getWarningCount(), + $this->fileReports, + )); } - /** @return array */ - public function getSniffTimes(): array + // not implemented + public function getTotalInfoLevelViolationCount(): int + { + return array_sum(array_map( + static fn(FileReport $fileReport): int => $fileReport->getInfoCount(), + $this->fileReports, + )); + } + + public function hasFinalViolations(): bool + { + return $this->getTotalFinalViolationCount() > 0; + } + + /** @return list */ + public function getAllViolations(): array { - return $this->sniffTimes; + $violations = []; + + foreach ($this->fileReports as $fileReport) { + if (!$fileReport->hasFinalViolations()) { + continue; + } + + array_push($violations, ...$fileReport->finalViolations); + } + + return $violations; } } diff --git a/src/Report/ReportException.php b/src/Report/ReportException.php new file mode 100644 index 0000000..07e0d36 --- /dev/null +++ b/src/Report/ReportException.php @@ -0,0 +1,18 @@ +appendChild($root); $comment = $dom->createComment( - sprintf(' total runtime: %.3fs ', $report->getTotalTime()) + sprintf(' total runtime: %.3fs ', $report->totalTime) ); $root->appendChild($comment); - foreach ($report->getFileReports() as $fileReport) { - if (!$fileReport->hasViolations()) { + foreach ($report->fileReports as $fileReport) { + if (!$fileReport->hasFinalViolations()) { continue; } $fileNode = $dom->createElement('file'); $fileNode->setAttribute('name', RelativePath::fromWorkingDirectory($fileReport->filePath)); - foreach ($fileReport->getViolations() as $violation) { + foreach ($fileReport->finalViolations as $violation) { $errorNode = $dom->createElement('error'); $errorNode->setAttribute('line', (string) $violation->rangeOne()->line); $errorNode->setAttribute('severity', $violation->severity->value); diff --git a/src/Report/Reporter/ConsoleReporter.php b/src/Report/Reporter/ConsoleReporter.php index c7fdb1e..eb0b8b4 100644 --- a/src/Report/Reporter/ConsoleReporter.php +++ b/src/Report/Reporter/ConsoleReporter.php @@ -23,8 +23,8 @@ public function generate(Report $report): string { $output = ''; - foreach ($report->getFileReports() as $fileReport) { - if (!$fileReport->hasViolations()) { + foreach ($report->fileReports as $fileReport) { + if (!$fileReport->hasFinalViolations()) { continue; } @@ -34,7 +34,7 @@ public function generate(Report $report): string $output .= $this->bold('FILE: ' . $filePath) . PHP_EOL; $output .= str_repeat('-', min(80, 6 + strlen($filePath))) . PHP_EOL; - foreach ($fileReport->getViolations() as $violation) { + foreach ($fileReport->finalViolations as $violation) { $output .= sprintf( ' %4d | %s | %s | %s', $violation->rangeOne()->line, @@ -60,75 +60,151 @@ public function generate(Report $report): string private function buildSummary(Report $report): string { - $files = $report->getFilesScanned(); - $errors = $report->getTotalErrors(); - $warnings = $report->getTotalWarnings(); - $total = $report->getTotalViolations(); - $time = $report->getTotalTime(); - - $timeLine = sprintf('Total runtime: %.3fs', $time); - - if ($total === 0) { - return $this->green( - sprintf( - 'OK -- %d file(s) scanned, no violations found.', - $files, - ) - ) . PHP_EOL . $this->dim($timeLine); + $lines = $report->hasFixingResults() + ? $this->buildFixingOutcome($report) + : [$this->buildSniffingOutcome($report)]; + + $lines[] = $this->dim(sprintf('Total runtime: %.3fs', $report->totalTime)); + + return implode(PHP_EOL, $lines); + } + + /** @return non-empty-list */ + private function buildFixingOutcome(Report $report): array + { + $lines = [ + $this->green($this->formatFixedSummary($report)), + ]; + + if ($report->hasFinalViolations()) { + $lines[] = $this->red($this->formatFinalSummary($report, 'REMAINING')); + } + + return $lines; + } + + private function buildSniffingOutcome(Report $report): string + { + if ($report->hasFinalViolations()) { + return $this->red($this->formatFinalSummary($report, 'FOUND')); } - return $this->red( - sprintf( - 'FOUND %d violation(s) (%d error(s), %d warning(s)) in %d file(s).', - $total, - $errors, - $warnings, - count($report->getFileReports()), - ) - ) . PHP_EOL . $this->dim($timeLine); + return $this->green(sprintf( + 'OK -- %s scanned, no violations found.', + $this->formatCount('file', $report->getScannedFilesCount()), + )); } private function buildPerformance(Report $report): string { - $totalTime = $report->getTotalTime(); - $sniffTimes = $report->getSniffTimes(); + $totalTime = $report->totalTime; + $rows = $this->collectPerformanceRows($report); - if ($totalTime <= 0.0 || $sniffTimes === []) { + if ($totalTime <= 0.0 || $rows === []) { return $this->dim('No performance data available.'); } - // Sort slowest first - arsort($sniffTimes); - - $output = $this->bold('PERFORMANCE') . PHP_EOL; - $output .= str_repeat('-', 40) . PHP_EOL; + $nameWidth = 40; + foreach (array_keys($rows) as $sniffCode) { + $nameWidth = max($nameWidth, strlen($sniffCode)); + } - $output .= sprintf( - ' Total runtime: %.3fs', - $totalTime - ) . PHP_EOL . PHP_EOL; + $header = sprintf(' %-*s %16s %16s', $nameWidth, '', 'Sniffing', 'Fixing'); + $lines = [$this->bold('PERFORMANCE'), str_repeat('-', strlen($header)), $this->bold($header)]; + + foreach ($rows as $sniffCode => $times) { + $lines[] = sprintf( + ' %-*s %16s %16s', + $nameWidth, + $sniffCode, + $this->formatPerformanceCell($times['sniffing'], $totalTime), + $this->formatPerformanceCell($times['fixing'], $totalTime), + ); + } - foreach ($sniffTimes as $sniff => $time) { - $percent = ($time / $totalTime) * 100; + return implode(PHP_EOL, $lines); + } - $output .= sprintf( - ' %-40s %6.3fs (%5.1f%%)', - $sniff, - $time, - $percent, - ) . PHP_EOL; + /** @return array */ + private function collectPerformanceRows(Report $report): array + { + $sniffingTimes = $report->getSniffingTimes(); + $fixingTimes = $report->getFixingTimes(); + $rows = []; + + foreach ($sniffingTimes + $fixingTimes as $sniffCode => $_) { + $rows[$sniffCode] = [ + 'sniffing' => $sniffingTimes[$sniffCode] ?? null, + 'fixing' => $fixingTimes[$sniffCode] ?? null, + ]; } - return $output; + // Sort slowest first. + uasort( + $rows, + static fn(array $left, array $right): int => + (($right['sniffing'] ?? 0.0) + ($right['fixing'] ?? 0.0)) + <=> (($left['sniffing'] ?? 0.0) + ($left['fixing'] ?? 0.0)), + ); + + return $rows; } private function formatSeverity(Severity $severity): string { - return match ($severity) { // @codeCoverageIgnore + return match ($severity) { Severity::ERROR => $this->red(str_pad(Severity::ERROR->name, 7)), Severity::WARNING => $this->yellow(str_pad(Severity::WARNING->name, 7)), default => $this->dim(str_pad(strtoupper($severity->name), 7)), - }; // @codeCoverageIgnore + }; + } + + private function formatPerformanceCell(?float $time, float $totalTime): string + { + return $time === null + ? '' + : sprintf('%6.3fs (%5.1f%%)', $time, ($time / $totalTime) * 100); + } + + private function formatFixedSummary(Report $report): string + { + $files = $report->getChangedFilesCount(); + $passes = $report->getFixingPassesCount(); + $ending = $report->hasFinalViolations() + ? '.' + : ', no violations remaining.'; + + return sprintf( + 'FIXED %s [%s, %s] in %s%s%s', + $this->formatCount('violation', $report->getAppliedFixesCount()), + $this->formatCount('error', $report->getFixedErrorCount()), + $this->formatCount('warning', $report->getFixedWarningCount()), + $this->formatCount('file', $files), + $files > 0 && $passes > $files + ? sprintf(' (%s passes)', number_format($passes)) + : '', + $ending, + ); + } + + private function formatFinalSummary(Report $report, string $label): string + { + // todo: how about info level? + return sprintf( + '%s %s [%s, %s] in %s.', + $label, + $this->formatCount('violation', $report->getTotalFinalViolationCount()), + $this->formatCount('error', $report->getTotalErrorLevelViolationCount()), + $this->formatCount('warning', $report->getTotalWarningLevelViolationCount()), + $this->formatCount('file', $report->getViolatingFilesCount()), + ); + } + + private function formatCount(string $singular, int $count): string + { + $suffix = $count === 1 ? $singular : $singular . 's'; + + return sprintf('%s %s', number_format($count), $suffix); } private function bold(string $text): string diff --git a/src/Report/Reporter/JsonReporter.php b/src/Report/Reporter/JsonReporter.php index eac2dd5..42aeb96 100644 --- a/src/Report/Reporter/JsonReporter.php +++ b/src/Report/Reporter/JsonReporter.php @@ -13,24 +13,30 @@ public function generate(Report $report): string { $data = [ 'totals' => [ - 'files_scanned' => $report->getFilesScanned(), - 'violations' => $report->getTotalViolations(), - 'errors' => $report->getTotalErrors(), - 'warnings' => $report->getTotalWarnings(), + 'files_scanned' => $report->getScannedFilesCount(), + 'violations' => $report->getTotalFinalViolationCount(), + 'errors' => $report->getTotalErrorLevelViolationCount(), + 'warnings' => $report->getTotalWarningLevelViolationCount(), ], 'files' => [], + 'fixing' => [ + 'files_changed' => $report->getChangedFilesCount(), + 'fixes_applied' => $report->getAppliedFixesCount(), + 'fixes_skipped' => $report->getSkippedFixesCount(), + 'fixing_passes' => $report->getFixingPassesCount(), + ], 'performance' => [ - 'total_runtime_seconds' => $report->getTotalTime(), + 'total_runtime_seconds' => $report->totalTime, ], ]; - foreach ($report->getFileReports() as $fileReport) { - if (!$fileReport->hasViolations()) { + foreach ($report->fileReports as $fileReport) { + if (!$fileReport->hasFinalViolations()) { continue; } $violations = []; - foreach ($fileReport->getViolations() as $violation) { + foreach ($fileReport->finalViolations as $violation) { $violations[] = [ 'line' => $violation->rangeOne()->line, 'severity' => $violation->severity, diff --git a/src/Runner/RunCoordinator.php b/src/Runner/RunCoordinator.php index c2de7ff..c17b0de 100644 --- a/src/Runner/RunCoordinator.php +++ b/src/Runner/RunCoordinator.php @@ -8,8 +8,8 @@ use DocbookCS\Fix\FixerException; use DocbookCS\Progress\NullProgress; use DocbookCS\Progress\ProgressInterface; -use DocbookCS\Report\FileReport; use DocbookCS\Report\Report; +use DocbookCS\Report\ReportException; use DocbookCS\Sniff\SniffInterface; use DocbookCS\Source\File; use DocbookCS\Violation\Violation; @@ -18,60 +18,70 @@ final class RunCoordinator { private ProgressInterface $progress; - public function __construct(?ProgressInterface $progress = null) - { + public function __construct( + ?ProgressInterface $progress = null, + private readonly bool $collectPerformance = false, + ) { $this->progress = $progress ?? new NullProgress(); } /** * @throws \InvalidArgumentException if an internal violation is inconsistent - * @throws \RuntimeException if a sniff class cannot be found or does not - * implement SniffInterface. + * @throws ReportException if violations are added in an invalid order + * @throws \RuntimeException if a sniff class cannot be found or does not implement SniffInterface. * @throws FixerException */ - public function run(RunPlan $plan): Report + public function runWithMetrics(RunPlan $plan): Report { - $startTime = microtime(true); + $report = new Report($this->collectPerformance); - $sniffs = $this->instantiateSniffs($plan->sniffs, $plan->mode); + return $report->measureWallTime( + fn(): Report => $this->run($plan, $report), + ); + } - $report = new Report(); - $preprocessor = new EntityPreprocessor($plan->entities); - $processor = new XmlFileProcessor($sniffs, $preprocessor, $report); + /** + * @throws \InvalidArgumentException if an internal violation is inconsistent + * @throws ReportException if violations are added in an invalid order + * @throws \RuntimeException if a sniff class cannot be found or does not implement SniffInterface. + * @throws FixerException + */ + private function run(RunPlan $plan, Report $report): Report + { + $processor = new XmlFileProcessor(new XmlSniffRunner( + $plan->mode, + $this->instantiateSniffs($plan->sniffs), + new EntityPreprocessor($plan->entities), + )); $this->progress->start(count($plan->targets)); foreach ($plan->targets as $filePath => $fileChange) { - $report->incrementFilesScanned(); + $fileReport = $report->newFileReport($filePath); $content = @file_get_contents($filePath); if ($content === false) { - $fileReport = new FileReport($filePath); - $fileReport->addViolation(Violation::fromFileReadFailure($filePath)); - } else { - $file = new File($filePath, $content); - $result = $processor->process($file, $fileChange); - $fileReport = $result->fileReport; - - if ($result->isModified() && @file_put_contents($filePath, $result->fixedContent()) === false) { - throw FixerException::cannotPersist($filePath); - } + $fileReport->addFailedViolation(Violation::fromFileReadFailure($filePath)); + + $this->progress->advance($filePath, $fileReport->getFinalViolationCount()); + continue; } - $violationCount = $fileReport->getViolationCount(); + $file = new File($filePath, $content); + $scope = RunScope::fromFileAndFileChange($file, $fileChange); + + $fixedFile = $processor->process($file, $fileReport, $scope); - if ($fileReport->hasViolations()) { - $report->addFileReport($fileReport); + if ($fixedFile !== null && @file_put_contents($filePath, $fixedFile->content) === false) { + throw FixerException::cannotPersist($filePath); } - $this->progress->advance($filePath, $violationCount); + $this->progress->advance($filePath, $fileReport->getFinalViolationCount()); } $this->progress->finish(); - $report->setTotalTime(microtime(true) - $startTime); - return $report; } @@ -80,7 +90,7 @@ public function run(RunPlan $plan): Report * @return list * @throws \RuntimeException if a sniff class cannot be found or does not implement SniffInterface. */ - private function instantiateSniffs(array $entries, RunMode $mode): array + private function instantiateSniffs(array $entries): array { $sniffs = []; @@ -93,7 +103,7 @@ private function instantiateSniffs(array $entries, RunMode $mode): array ); } - $instance = new $className($mode); + $instance = new $className(); if (!$instance instanceof SniffInterface) { throw new \RuntimeException( diff --git a/src/Runner/RunScopeResolver.php b/src/Runner/RunScopeResolver.php index 89efb53..dd57aca 100644 --- a/src/Runner/RunScopeResolver.php +++ b/src/Runner/RunScopeResolver.php @@ -105,17 +105,10 @@ private function absolutePaths(array $paths): array private function expandReferencedTargets(array &$targets): void { $pending = array_keys($targets); - $visitedFiles = []; $visitedEntityPaths = []; for ($i = 0; isset($pending[$i]); $i++) { $file = $pending[$i]; - - if (isset($visitedFiles[$file])) { - continue; - } - - $visitedFiles[$file] = true; $content = @file_get_contents($file); if ($content === false) { diff --git a/src/Runner/XmlFileProcessor.php b/src/Runner/XmlFileProcessor.php index dd5e2ce..477c77c 100644 --- a/src/Runner/XmlFileProcessor.php +++ b/src/Runner/XmlFileProcessor.php @@ -4,15 +4,9 @@ namespace DocbookCS\Runner; -use DocbookCS\Diff\FileChange; -use DocbookCS\Fix\Fix; -use DocbookCS\Fix\FixApplier; -use DocbookCS\Fix\FixPlan; use DocbookCS\Fix\FixerException; use DocbookCS\Report\FileReport; -use DocbookCS\Report\Report; -use DocbookCS\Sniff\Fixable; -use DocbookCS\Sniff\SniffInterface; +use DocbookCS\Report\ReportException; use DocbookCS\Source\File; use DocbookCS\Violation\Violation; @@ -20,143 +14,70 @@ { private const int MAX_FIX_PASSES = 20; - /** @var list */ - private array $sniffs; - - private EntityPreprocessor $preprocessor; - - private Report $report; - - private ViolationScopeFilter $violationScopeFilter; - - /** @param list $sniffs */ public function __construct( - array $sniffs, - ?EntityPreprocessor $preprocessor = null, - ?Report $report = null, + private XmlSniffRunner $xmlSniffRunner, + private XmlFixRunner $xmlFixRunner = new XmlFixRunner(), ) { - $this->sniffs = $sniffs; - $this->preprocessor = $preprocessor ?? new EntityPreprocessor([]); - $this->report = $report ?? new Report(); - $this->violationScopeFilter = new ViolationScopeFilter(); } /** * @throws FixerException * @throws \InvalidArgumentException if an internal violation is inconsistent + * @throws ReportException if violations are added in an invalid order */ - public function process(File $initialFile, ?FileChange $fileChange = null): XmlProcessingResult + public function process(File $file, FileReport $fileReport, RunScope $scope): ?File { - $fileReport = new FileReport($initialFile->path); - $currentFile = $initialFile; - $scope = RunScope::fromFileAndFileChange($initialFile, $fileChange); - $seenContentHashes = [hash('sha256', $currentFile->content) => true]; - $fixPasses = 0; + $seenContentHashes = [hash('sha256', $file->content) => true]; + $initialViolations = null; + $changed = false; while (true) { - $passReport = new FileReport($currentFile->path); + $sniffingResult = $this->xmlSniffRunner->runWithMetrics($file, $fileReport, $scope); - $document = $this->parseXml($currentFile, $passReport); - if ($document === null) { - if ($currentFile->content !== $initialFile->content) { - throw FixerException::invalidFixedXml($currentFile->path); + if ($sniffingResult instanceof \LibXMLError) { + if ($changed) { + throw FixerException::invalidFixedXml($file->path); } - break; + $fileReport->addFailedViolation(Violation::fromXmlParseError($file->path, $sniffingResult)); + return null; } - $fixes = $this->runSniffs($document, $currentFile, $passReport, $scope); + [$passViolations, $fixerBatches] = $sniffingResult; + + $initialViolations ??= $passViolations; - if ($fixes === []) { + if ($fixerBatches === []) { break; } - $fixResult = new FixApplier()->apply($currentFile, $fixes); + $fixResult = $this->xmlFixRunner->runWithMetrics($file, $fileReport, $fixerBatches); if ($fixResult->applied === 0) { break; } - $fixPasses++; $fixedContentHash = hash('sha256', $fixResult->file->content); - if ( - $fixPasses > self::MAX_FIX_PASSES - || $fixResult->file->content === $currentFile->content - || isset($seenContentHashes[$fixedContentHash]) - ) { - throw FixerException::didNotConverge($currentFile->path); + if ($fileReport->fixingPasses > self::MAX_FIX_PASSES || isset($seenContentHashes[$fixedContentHash])) { + throw FixerException::didNotConverge($file->path); } $seenContentHashes[$fixedContentHash] = true; $scope = $scope->after($fixResult->appliedFixes); - $currentFile = $fixResult->file; - } - - $fileReport->addViolations($passReport->getViolations()); - - return new XmlProcessingResult( - fileReport: $fileReport, - initialFile: $initialFile, - currentFile: $currentFile, - ); - } - - /** - * @return list - * @throws FixerException - */ - private function runSniffs(\DOMDocument $document, File $file, FileReport $fileReport, RunScope $scope): array - { - $fixes = []; - - foreach ($this->sniffs as $sniff) { - $start = microtime(true); - - $sniffViolations = $sniff->process($document, $file); - - $this->report->addSniffTime($sniff::getCode(), microtime(true) - $start); - - $relevantViolations = $this->violationScopeFilter->filter($sniffViolations, $document, $file, $scope); - - $fileReport->addViolations($relevantViolations); - - if (!$sniff->mode->isFixMode() || !$sniff instanceof Fixable) { - continue; - } - - $fixer = new ($sniff::getFixerClassName()); - - foreach ($relevantViolations as $violation) { - $fixes[] = $fixer->process($violation); - } + $file = $fixResult->file; + $changed = true; } - return $fixes; - } - - /** @throws \InvalidArgumentException if an internal violation is inconsistent */ - private function parseXml(File $file, FileReport $fileReport): ?\DOMDocument - { - $content = $this->preprocessor->processForParsing($file->content); - - $previousUseErrors = libxml_use_internal_errors(true); - $document = new \DOMDocument(); - $document->preserveWhiteSpace = true; - - // LIBXML_NONET prevents network access. - // No LIBXML_DTDLOAD needed since we stripped the DOCTYPE. - $loaded = $document->loadXML($content, LIBXML_NONET); + $fileReport->addFoundViolations($initialViolations); - $errors = libxml_get_errors(); - libxml_clear_errors(); - libxml_use_internal_errors($previousUseErrors); - - if (!$loaded) { - $fileReport->addViolation(Violation::fromXmlParseError($file->path, $errors[0] ?? null)); + if (!$changed) { return null; } - return $document; + $fileReport->addFinalViolations($passViolations); + $fileReport->markChanged(); + + return $file; } } diff --git a/src/Runner/XmlFixRunner.php b/src/Runner/XmlFixRunner.php new file mode 100644 index 0000000..d8d9aab --- /dev/null +++ b/src/Runner/XmlFixRunner.php @@ -0,0 +1,63 @@ +, + * violations: list + * }> $fixerBatches + */ + public function runWithMetrics(File $file, FileReport $fileReport, array $fixerBatches): FixResult + { + return $fileReport->measureFixing( + fn(): FixResult => $this->run($file, $fileReport, $fixerBatches), + ); + } + + /** + * @param list, + * violations: list + * }> $fixerBatches + */ + private function run(File $file, FileReport $fileReport, array $fixerBatches): FixResult + { + $fixes = []; + $fileReport->recordFixingPass(); + + foreach ($fixerBatches as $batch) { + $batchFixes = $fileReport->measureFixer( + $batch['sniffCode'], + static function () use ($batch): array { + $fixes = []; + $fixerClass = $batch['fixerClass']; + $fixer = new $fixerClass(); + + foreach ($batch['violations'] as $violation) { + $fixes[] = $fixer->process($violation); + } + + return $fixes; + }, + ); + + array_push($fixes, ...$batchFixes); + } + + return new FixApplier()->apply($file, $fixes); + } +} diff --git a/src/Runner/XmlProcessingResult.php b/src/Runner/XmlProcessingResult.php deleted file mode 100644 index a586e70..0000000 --- a/src/Runner/XmlProcessingResult.php +++ /dev/null @@ -1,34 +0,0 @@ -initialFile->content !== $this->currentFile->content; - } - - /** @throws FixerException */ - public function fixedContent(): string - { - if (!$this->isModified()) { - throw FixerException::cannotReadFixedContent(); - } - - return $this->currentFile->content; - } -} diff --git a/src/Runner/XmlSniffRunner.php b/src/Runner/XmlSniffRunner.php new file mode 100644 index 0000000..d8361d9 --- /dev/null +++ b/src/Runner/XmlSniffRunner.php @@ -0,0 +1,101 @@ + $sniffs */ + public function __construct( + private RunMode $mode, + private array $sniffs, + private EntityPreprocessor $preprocessor = new EntityPreprocessor(), + private XmlParser $xmlParser = new XmlParser(), + private ViolationScopeFilter $violationFilter = new ViolationScopeFilter(), + ) { + } + + /** + * @return array{ + * list, + * list, + * violations: list + * }> + * }|\LibXMLError + * @throws \InvalidArgumentException if an internal violation is inconsistent + */ + public function runWithMetrics(File $file, FileReport $fileReport, RunScope $scope): array|\LibXMLError + { + return $fileReport->measureSniffing( + fn(): array|\LibXMLError => $this->run($file, $fileReport, $scope), + ); + } + + /** + * @return array{ + * list, + * list, + * violations: list + * }> + * }|\LibXMLError + * @throws \InvalidArgumentException if an internal violation is inconsistent + */ + private function run(File $file, FileReport $fileReport, RunScope $scope): array|\LibXMLError + { + if (($document = $this->parseXml($file)) instanceof \LibXMLError) { + return $document; + } + + $violations = []; + /** @var list, violations: list}> $fixerBatches */ + $fixerBatches = []; + + foreach ($this->sniffs as $sniffer) { + $sniffViolations = $fileReport->measureSniffer( + $sniffer::getCode(), + // run sniffers and filter violations + fn() => $this->violationFilter->filter( + $sniffer->process($document, $file), + $document, + $file, + $scope, + ), + ); + + array_push($violations, ...$sniffViolations); + + if ($sniffViolations === [] || !$this->mode->isFixMode() || !$sniffer instanceof Fixable) { + continue; + } + + $fixerBatches[] = [ + 'sniffCode' => $sniffer::getCode(), + 'fixerClass' => $sniffer::getFixerClassName(), + 'violations' => $sniffViolations, + ]; + } + + return [$violations, $fixerBatches]; + } + + /** @throws \InvalidArgumentException if the internal violation is inconsistent */ + private function parseXml(File $file): \DOMDocument|\LibXMLError + { + $content = $this->preprocessor->processForParsing($file->content); + + return $this->xmlParser->parseDocument($content); + } +} diff --git a/src/Sniff/AbstractSniff.php b/src/Sniff/AbstractSniff.php index eaa4147..25763f2 100644 --- a/src/Sniff/AbstractSniff.php +++ b/src/Sniff/AbstractSniff.php @@ -5,7 +5,6 @@ namespace DocbookCS\Sniff; use DocbookCS\Runner\EntityExpansionMarker; -use DocbookCS\Runner\RunMode; use DocbookCS\Source\File; use DocbookCS\Violation\Severity; use DocbookCS\Violation\SourceRange; @@ -17,22 +16,11 @@ */ abstract class AbstractSniff implements SniffInterface { - private const array NON_ELEMENT_DELIMITERS = [ - '', - ' ']]>', - ' '?>', - ]; - protected Severity $severity = Severity::ERROR; /** @var array */ protected array $properties = []; - public function __construct( - public RunMode $mode = RunMode::Sniff, - ) { - } - /** @throws \InvalidArgumentException if a configured severity is invalid */ public function setProperty(string $name, string $value): void { @@ -108,89 +96,4 @@ protected function createViolation( fixerData: $fixerData, ); } - - protected function maskNonElementMarkup(string $source): string - { - $masked = $source; - $offset = 0; - - while (false !== $start = strpos($source, '<', $offset)) { - $endOffset = $this->nonElementMarkupEndOffset($source, $start); - - if ($endOffset === null) { - $offset = $start + 1; - continue; - } - - for ($i = $start; $i < $endOffset; $i++) { - $masked[$i] = ' '; - } - - $offset = $endOffset; - } - - return $masked; - } - - private function nonElementMarkupEndOffset(string $source, int $start): ?int - { - foreach (self::NON_ELEMENT_DELIMITERS as $opening => $closing) { - if (substr_compare($source, $opening, $start, strlen($opening)) === 0) { - return $this->offsetAfterDelimiter($source, $closing, $start); - } - } - - if (substr_compare($source, 'declarationEndOffset($source, $start); - } - - return null; - } - - private function offsetAfterDelimiter(string $source, string $delimiter, int $offset): int - { - $end = strpos($source, $delimiter, $offset); - - return $end === false ? strlen($source) : $end + strlen($delimiter); - } - - private function declarationEndOffset(string $source, int $offset): int - { - $length = strlen($source); - $quote = null; - $bracketDepth = 0; - - for ($i = $offset; $i < $length; $i++) { - $character = $source[$i]; - - if ($quote !== null) { - if ($character === $quote) { - $quote = null; - } - - continue; - } - - if ($character === '"' || $character === "'") { - $quote = $character; - continue; - } - - if ($character === '[') { - $bracketDepth++; - continue; - } - - if ($character === ']') { - $bracketDepth--; - continue; - } - - if ($character === '>' && $bracketDepth === 0) { - return $i + 1; - } - } - - return $length; - } } diff --git a/src/Sniff/AttributeOrderSniff.php b/src/Sniff/AttributeOrderSniff.php index bc9182d..a8bec79 100644 --- a/src/Sniff/AttributeOrderSniff.php +++ b/src/Sniff/AttributeOrderSniff.php @@ -43,7 +43,7 @@ public function process(\DOMDocument $document, File $file): array // Match ONLY opening tags (skip closing, comments, xml decl) preg_match_all( self::OPENING_TAG_PATTERN, - $this->maskNonElementMarkup($file->content), + $file->contentWithNonElementMarkupMasked(), $matches, PREG_OFFSET_CAPTURE, ); diff --git a/src/Sniff/ExceptionNameSniff.php b/src/Sniff/ExceptionNameSniff.php index bcd9f19..cb13c68 100644 --- a/src/Sniff/ExceptionNameSniff.php +++ b/src/Sniff/ExceptionNameSniff.php @@ -17,8 +17,8 @@ */ final class ExceptionNameSniff extends AbstractSniff implements Fixable { - private const string REPORTING_MESSAGE = '"%s" is wrapped in but should use .'; private const string ELEMENT_NAME = 'classname'; + private const string REPORTING_MESSAGE = '"%s" is wrapped in but should use .'; /** * Default suffixes that indicate the class is an exception or error. @@ -121,7 +121,7 @@ private function sourceMatches(File $file): array { preg_match_all( self::CLASSNAME_PATTERN, - $this->maskNonElementMarkup($file->content), + $file->contentWithNonElementMarkupMasked(), $matches, PREG_OFFSET_CAPTURE, ); diff --git a/src/Sniff/MixedUnionSniff.php b/src/Sniff/MixedUnionSniff.php index 022c522..6af373f 100644 --- a/src/Sniff/MixedUnionSniff.php +++ b/src/Sniff/MixedUnionSniff.php @@ -30,7 +30,7 @@ public static function getFixerClassName(): string */ public function process(\DOMDocument $document, File $file): array { - $source = $this->maskNonElementMarkup($file->content); + $source = $file->contentWithNonElementMarkupMasked(); $synopsisRanges = $this->synopsisRanges($source); $violations = []; diff --git a/src/Sniff/NativeTypeSniff.php b/src/Sniff/NativeTypeSniff.php index 9f37c59..06254d8 100644 --- a/src/Sniff/NativeTypeSniff.php +++ b/src/Sniff/NativeTypeSniff.php @@ -55,7 +55,7 @@ public static function getFixerClassName(): string */ public function process(\DOMDocument $document, File $file): array { - $source = $this->maskNonElementMarkup($file->content); + $source = $file->contentWithNonElementMarkupMasked(); $synopsisRanges = $this->synopsisRanges($source); $redundantMixedUnions = array_values(array_filter( MixedUnionDetector::matches($source), diff --git a/src/Sniff/SimparaSniff.php b/src/Sniff/SimparaSniff.php index 0721d8f..8b2854c 100644 --- a/src/Sniff/SimparaSniff.php +++ b/src/Sniff/SimparaSniff.php @@ -144,7 +144,8 @@ public function process(\DOMDocument $document, File $file): array throw new \LogicException('Could not map simpara violation to source content.'); } - if ($match['selfClosing']) { + $closingOffset = $match['closingOffset']; + if (null === $closingOffset) { continue; } @@ -160,11 +161,6 @@ public function process(\DOMDocument $document, File $file): array continue; } - $closingOffset = $match['closingOffset']; - if ($closingOffset === null) { - throw new \LogicException('Could not map simpara violation to source content.'); - } - $affectedRanges = $this->elementNameRanges( $file, $match['beginOffset'], @@ -219,18 +215,12 @@ private function getAllowedElements(): array |> array_values(...); } - /** - * @return list - */ + /** @return list */ private function sourceMatches(File $file): array { preg_match_all( self::PARA_TAG_PATTERN, - $this->maskNonElementMarkup($file->content), + $file->contentWithNonElementMarkupMasked(), $matches, PREG_OFFSET_CAPTURE, ); @@ -245,7 +235,6 @@ private function sourceMatches(File $file): array if (str_ends_with(rtrim($tag), '/>')) { $sourceMatches[] = [ 'beginOffset' => $offset, - 'selfClosing' => true, 'closingOffset' => null, ]; continue; @@ -262,7 +251,6 @@ private function sourceMatches(File $file): array $sourceMatches[] = [ 'beginOffset' => $opening, - 'selfClosing' => false, 'closingOffset' => $offset, ]; } diff --git a/src/Sniff/SniffInterface.php b/src/Sniff/SniffInterface.php index d733afe..c29bbce 100644 --- a/src/Sniff/SniffInterface.php +++ b/src/Sniff/SniffInterface.php @@ -4,7 +4,6 @@ namespace DocbookCS\Sniff; -use DocbookCS\Runner\RunMode; use DocbookCS\Source\File; use DocbookCS\Violation\Violation; @@ -15,10 +14,6 @@ /** @template TFixerData = mixed */ interface SniffInterface { - public RunMode $mode { get; } - - public function __construct(RunMode $mode); - /** * Unique, human-readable code for this sniff (e.g. "DocbookCS.MySniff"). */ diff --git a/src/Source/File.php b/src/Source/File.php index 02f8be5..0ead9eb 100644 --- a/src/Source/File.php +++ b/src/Source/File.php @@ -6,9 +6,17 @@ final class File { + private const array NON_ELEMENT_DELIMITERS = [ + '', + ' ']]>', + ' '?>', + ]; + /** @var non-empty-list|null */ private ?array $lineBeginOffsets = null; + private ?string $maskedContent = null; + public function __construct( public readonly string $path, public readonly string $content, @@ -48,6 +56,33 @@ public function withContent(string $content): self : new self($this->path, $content); } + public function contentWithNonElementMarkupMasked(): string + { + if ($this->maskedContent !== null) { + return $this->maskedContent; + } + + $masked = $this->content; + $offset = 0; + + while (false !== $start = strpos($this->content, '<', $offset)) { + $endOffset = $this->nonElementMarkupEndOffset($start); + + if ($endOffset === null) { + $offset = $start + 1; + continue; + } + + for ($i = $start; $i < $endOffset; $i++) { + $masked[$i] = ' '; + } + + $offset = $endOffset; + } + + return $this->maskedContent = $masked; + } + /** @return non-empty-list */ private function lineBeginOffsets(): array { @@ -108,6 +143,68 @@ private function lineIndexAtOffset(int $offset, array $lineBeginOffsets): int return $low; } + private function nonElementMarkupEndOffset(int $start): ?int + { + foreach (self::NON_ELEMENT_DELIMITERS as $opening => $closing) { + if (substr_compare($this->content, $opening, $start, strlen($opening)) === 0) { + return $this->offsetAfterDelimiter($closing, $start); + } + } + + if (substr_compare($this->content, 'declarationEndOffset($start); + } + + return null; + } + + private function offsetAfterDelimiter(string $delimiter, int $offset): int + { + $end = strpos($this->content, $delimiter, $offset); + + return $end === false ? strlen($this->content) : $end + strlen($delimiter); + } + + private function declarationEndOffset(int $offset): int + { + $length = strlen($this->content); + $quote = null; + $bracketDepth = 0; + + for ($i = $offset; $i < $length; $i++) { + $character = $this->content[$i]; + + if ($quote !== null) { + if ($character === $quote) { + $quote = null; + } + + continue; + } + + if ($character === '"' || $character === "'") { + $quote = $character; + continue; + } + + if ($character === '[') { + $bracketDepth++; + continue; + } + + if ($character === ']') { + $bracketDepth--; + continue; + } + + if ($character === '>' && $bracketDepth === 0) { + return $i + 1; + } + } + + return $length; + } + private function createLineAtOffset(int $lineNumber, int $lineBeginOffset): Line { $sourceLength = strlen($this->content); diff --git a/src/Xml/XmlParser.php b/src/Xml/XmlParser.php new file mode 100644 index 0000000..8fd634d --- /dev/null +++ b/src/Xml/XmlParser.php @@ -0,0 +1,67 @@ +preserveWhiteSpace = true; + + [$loaded, $error] = $this->parseWithErrors( + static fn(): bool => $document->loadXML($content, LIBXML_NONET), + ); + + if ($loaded) { + return $document; + } + + return $this->parseError($error); + } + + public function parseElement(string $content): \SimpleXMLElement|\LibXMLError + { + [$element, $error] = $this->parseWithErrors( + static fn(): \SimpleXMLElement|false => simplexml_load_string($content), + ); + + if ($element !== false) { + return $element; + } + + return $this->parseError($error); + } + + /** + * @template TResult + * @param callable(): TResult $parser + * @return array{TResult, ?\LibXMLError} + */ + private function parseWithErrors(callable $parser): array + { + $previousUseErrors = libxml_use_internal_errors(true); + libxml_clear_errors(); + + try { + $result = $parser(); + $error = libxml_get_errors()[0] ?? null; + } finally { + libxml_clear_errors(); + libxml_use_internal_errors($previousUseErrors); + } + + return [$result, $error]; + } + + private function parseError(?\LibXMLError $error): \LibXMLError + { + $error ??= new \LibXMLError(); + $error->message ??= 'Unknown parse error'; + $error->line ??= 0; + + return $error; + } +} diff --git a/tests/Support/XmlHelper.php b/tests/Support/XmlHelper.php new file mode 100644 index 0000000..d8b92f5 --- /dev/null +++ b/tests/Support/XmlHelper.php @@ -0,0 +1,16 @@ + +$body +XML; + } +} diff --git a/tests/Unit/ApplicationInputTest.php b/tests/Unit/ApplicationInputTest.php index 3d7a272..cd15976 100644 --- a/tests/Unit/ApplicationInputTest.php +++ b/tests/Unit/ApplicationInputTest.php @@ -13,22 +13,39 @@ use DocbookCS\Diff\DiffParser; use DocbookCS\Diff\GitDiffProvider; use DocbookCS\Diff\UpstreamResolver; +use DocbookCS\Fix\Fix; +use DocbookCS\Fix\FixApplier; +use DocbookCS\Fix\FixPlan; +use DocbookCS\Fix\FixResult; +use DocbookCS\Fix\Fixer\ExceptionNameFixer; use DocbookCS\Git\GitClient; use DocbookCS\Git\GitException; use DocbookCS\Path\DiffPathLoader; use DocbookCS\Path\EntityResolver; +use DocbookCS\Path\PathLoader; use DocbookCS\Path\PathMatcher; use DocbookCS\Progress\NullProgress; +use DocbookCS\Report\FileReport; use DocbookCS\Report\Report; use DocbookCS\Report\Reporter\ConsoleReporter; +use DocbookCS\Runner\EntityExpansionMarker; use DocbookCS\Runner\EntityPreprocessor; use DocbookCS\Runner\RunCoordinator; use DocbookCS\Runner\RunMode; use DocbookCS\Runner\RunPlan; use DocbookCS\Runner\RunPlanner; +use DocbookCS\Runner\RunScope; use DocbookCS\Runner\RunScopeResolver; +use DocbookCS\Runner\ViolationScopeFilter; use DocbookCS\Runner\XmlFileProcessor; +use DocbookCS\Runner\XmlFixRunner; +use DocbookCS\Runner\XmlSniffRunner; use DocbookCS\Sniff\AbstractSniff; +use DocbookCS\Sniff\ExceptionNameSniff; +use DocbookCS\Source\File; +use DocbookCS\Violation\SourceRange; +use DocbookCS\Violation\Violation; +use DocbookCS\Xml\XmlParser; use PHPUnit\Framework\Attributes\CoversClass; use PHPUnit\Framework\Attributes\Test; use PHPUnit\Framework\Attributes\UsesClass; @@ -45,22 +62,39 @@ UsesClass(DiffChangeset::class), UsesClass(DiffParser::class), UsesClass(DiffPathLoader::class), + UsesClass(EntityExpansionMarker::class), UsesClass(EntityPreprocessor::class), UsesClass(EntityResolver::class), + UsesClass(ExceptionNameFixer::class), + UsesClass(ExceptionNameSniff::class), + UsesClass(File::class), + UsesClass(FileReport::class), + UsesClass(Fix::class), + UsesClass(FixApplier::class), + UsesClass(FixPlan::class), + UsesClass(FixResult::class), UsesClass(GitClient::class), UsesClass(GitDiffProvider::class), UsesClass(GitException::class), UsesClass(NullProgress::class), + UsesClass(PathLoader::class), UsesClass(PathMatcher::class), UsesClass(Report::class), UsesClass(RunCoordinator::class), UsesClass(RunMode::class), UsesClass(RunPlan::class), UsesClass(RunPlanner::class), + UsesClass(RunScope::class), UsesClass(RunScopeResolver::class), UsesClass(SniffEntry::class), + UsesClass(SourceRange::class), UsesClass(UpstreamResolver::class), + UsesClass(Violation::class), + UsesClass(ViolationScopeFilter::class), UsesClass(XmlFileProcessor::class), + UsesClass(XmlFixRunner::class), + UsesClass(XmlParser::class), + UsesClass(XmlSniffRunner::class), ] final class ApplicationInputTest extends TestCase { @@ -117,6 +151,36 @@ public function itDetectsAPipedDiffWithoutAFlag(): void self::assertSame('', $this->readStream($this->stderr)); } + #[Test] + public function itDetectsPipedDiffThroughCliEntryPoint(): void + { + $process = proc_open( + [PHP_BINARY, __DIR__ . '/../../bin/docbook-cs', '--config=' . self::VALID_CONFIG, self::SCAN_FILE], + [ + ['pipe', 'r'], + ['pipe', 'w'], + ['pipe', 'w'], + ], + $pipes, + getcwd() ?: '.', + ); + self::assertIsResource($process); + self::assertCount(3, $pipes); + self::assertIsResource($pipes[0]); + self::assertIsResource($pipes[1]); + self::assertIsResource($pipes[2]); + + fclose($pipes[0]); + $stdout = stream_get_contents($pipes[1]); + $stderr = stream_get_contents($pipes[2]); + fclose($pipes[1]); + fclose($pipes[2]); + + self::assertSame(2, proc_close($process), $stdout ?: ''); + self::assertIsString($stderr); + self::assertStringContainsString('Paths cannot be combined with diff input', $stderr); + } + #[Test] public function itRejectsPathsCombinedWithAPipedDiff(): void { @@ -158,6 +222,32 @@ public function itAcceptsTheWideOption(): void self::assertSame('', $this->readStream($this->stderr)); } + #[Test] + public function itAppliesFixesInFixMode(): void + { + $temporaryPath = tempnam(sys_get_temp_dir(), 'docbook-cs-'); + self::assertIsString($temporaryPath); + $filePath = $temporaryPath . '.xml'; + rename($temporaryPath, $filePath); + file_put_contents($filePath, 'RuntimeException'); + + try { + $app = new Application( + ['docbook-cs', '--config=' . self::VALID_CONFIG, '--fix', $filePath], + $this->stdout, + $this->stderr, + ); + + self::assertSame(0, $app->run()); + self::assertSame( + 'RuntimeException', + file_get_contents($filePath), + ); + } finally { + @unlink($filePath); + } + } + /** @param resource $stream */ private function readStream(mixed $stream): string { diff --git a/tests/Unit/ApplicationTest.php b/tests/Unit/ApplicationTest.php index bc581fd..4deb4f1 100644 --- a/tests/Unit/ApplicationTest.php +++ b/tests/Unit/ApplicationTest.php @@ -30,17 +30,18 @@ use DocbookCS\Report\Reporter\ConsoleReporter; use DocbookCS\Report\Reporter\JsonReporter; use DocbookCS\Runner\EntityPreprocessor; -use DocbookCS\Runner\RunMode; +use DocbookCS\Runner\XmlFileProcessor; use DocbookCS\Runner\RunCoordinator; +use DocbookCS\Runner\RunMode; use DocbookCS\Runner\RunPlan; use DocbookCS\Runner\RunPlanner; use DocbookCS\Runner\RunScope; use DocbookCS\Runner\RunScopeResolver; use DocbookCS\Runner\ViolationScopeFilter; -use DocbookCS\Runner\XmlFileProcessor; -use DocbookCS\Runner\XmlProcessingResult; +use DocbookCS\Runner\XmlSniffRunner; use DocbookCS\Sniff\ExceptionNameSniff; use DocbookCS\Source\File; +use DocbookCS\Xml\XmlParser; use PHPUnit\Framework\Attributes\CoversClass; use PHPUnit\Framework\Attributes\Test; use PHPUnit\Framework\Attributes\UsesClass; @@ -68,7 +69,7 @@ CoversClass(RunPlan::class), CoversClass(RunPlanner::class), CoversClass(SniffEntry::class), - CoversClass(XmlFileProcessor::class), + CoversClass(XmlSniffRunner::class), // UsesClass(DiffBaseResolver::class), UsesClass(DiffChangeset::class), @@ -84,7 +85,8 @@ UsesClass(RunScopeResolver::class), UsesClass(UpstreamResolver::class), UsesClass(ViolationScopeFilter::class), - UsesClass(XmlProcessingResult::class), + UsesClass(XmlFileProcessor::class), + UsesClass(XmlParser::class), ] final class ApplicationTest extends TestCase { @@ -120,7 +122,7 @@ private function readStream(mixed $stream): string return stream_get_contents($stream) ?: ''; } - #[Test] // TODO: should be feature + #[Test] public function itPrintsHelpAndExitsWithZero(): void { $app = new Application(['docbook-cs', '--help'], $this->stdout, $this->stderr); @@ -132,7 +134,7 @@ public function itPrintsHelpAndExitsWithZero(): void self::assertSame('', $this->readStream($this->stderr)); } - #[Test] // TODO: should be feature + #[Test] public function itPrintsVersionAndExitsWithZero(): void { $app = new Application(['docbook-cs', '--version'], $this->stdout, $this->stderr); @@ -144,7 +146,7 @@ public function itPrintsVersionAndExitsWithZero(): void self::assertSame('', $this->readStream($this->stderr)); } - #[Test] // TODO: should be feature + #[Test] public function itReturnsErrorWhenConfigCannotBeLoaded(): void { $app = new Application( @@ -159,7 +161,7 @@ public function itReturnsErrorWhenConfigCannotBeLoaded(): void self::assertStringContainsString('Error:', $this->readStream($this->stderr)); } - #[Test] // TODO: should be feature + #[Test] public function itHandlesSeparateConfigArgument(): void { $app = new Application( @@ -174,7 +176,7 @@ public function itHandlesSeparateConfigArgument(): void self::assertStringContainsString('Error:', $this->readStream($this->stderr)); } - #[Test] // TODO: should be feature + #[Test] public function itAcceptsPathsWithoutCrashing(): void { $app = new Application( @@ -188,7 +190,7 @@ public function itAcceptsPathsWithoutCrashing(): void self::assertContains($exitCode, [0, 1, 2]); } - #[Test] // TODO: should be feature + #[Test] public function itSupportsQuietFlag(): void { $app = new Application(['docbook-cs', '--quiet'], $this->stdout, $this->stderr); @@ -198,7 +200,7 @@ public function itSupportsQuietFlag(): void self::assertContains($exitCode, [0, 1, 2]); } - #[Test] // TODO: should be feature + #[Test] public function itSupportsReportFormats(): void { foreach (['console', 'json', 'checkstyle'] as $format) { @@ -214,7 +216,7 @@ public function itSupportsReportFormats(): void } } - #[Test] // TODO: should be feature + #[Test] public function itSupportsColorFlags(): void { foreach (['--colors', '--no-colors'] as $flag) { @@ -230,7 +232,7 @@ public function itSupportsColorFlags(): void } } - #[Test] // TODO: should be feature + #[Test] public function helpShortCircuitsExecution(): void { $app = new Application( @@ -246,7 +248,7 @@ public function helpShortCircuitsExecution(): void self::assertSame('', $this->readStream($this->stderr)); } - #[Test] // TODO: should be feature + #[Test] public function versionShortCircuitsExecution(): void { $app = new Application( @@ -262,7 +264,7 @@ public function versionShortCircuitsExecution(): void self::assertSame('', $this->readStream($this->stderr)); } - #[Test] // TODO: should be feature + #[Test] public function itResolvesRelativeOverridePathsAgainstCwd(): void { $app = new Application( @@ -278,7 +280,7 @@ public function itResolvesRelativeOverridePathsAgainstCwd(): void self::assertNotSame(2, $exitCode); } - #[Test] // TODO: should be feature + #[Test] public function itCatchesRuntimeErrorFromRunner(): void { $app = new Application( @@ -293,7 +295,7 @@ public function itCatchesRuntimeErrorFromRunner(): void self::assertStringContainsString('Runtime error:', $this->readStream($this->stderr)); } - #[Test] // TODO: should be feature + #[Test] public function itSupportsSeparateReportArgument(): void { $app = new Application( @@ -307,7 +309,7 @@ public function itSupportsSeparateReportArgument(): void self::assertContains($exitCode, [0, 1, 2]); } - #[Test] // TODO: should be feature + #[Test] public function itPassesThroughAbsoluteOverridePaths(): void { $app = new Application( @@ -321,7 +323,7 @@ public function itPassesThroughAbsoluteOverridePaths(): void self::assertNotSame(2, $exitCode); } - #[Test] // TODO: should be feature + #[Test] public function itSuppressesProgressWhenQuietFlagIsSet(): void { $app = new Application( @@ -336,7 +338,7 @@ public function itSuppressesProgressWhenQuietFlagIsSet(): void self::assertSame('', $this->readStream($this->stderr)); } - #[Test] // TODO: should be feature + #[Test] public function itSuppressesProgressForStructuredReportFormats(): void { foreach (['json', 'checkstyle'] as $format) { @@ -359,7 +361,7 @@ public function itSuppressesProgressForStructuredReportFormats(): void } } - #[Test] // TODO: should be feature + #[Test] public function itShowsPerformanceWhenPerfFlagIsEnabled(): void { $app = new Application( @@ -382,7 +384,7 @@ public function itShowsPerformanceWhenPerfFlagIsEnabled(): void self::assertStringContainsString('PERFORMANCE', $output); } - #[Test] // TODO: should be feature + #[Test] public function itDoesNotShowPerformanceByDefault(): void { $app = new Application( diff --git a/tests/Unit/Config/ConfigParserTest.php b/tests/Unit/Config/ConfigParserTest.php index 1201e20..58db94a 100644 --- a/tests/Unit/Config/ConfigParserTest.php +++ b/tests/Unit/Config/ConfigParserTest.php @@ -8,8 +8,10 @@ use DocbookCS\Config\ConfigParser; use DocbookCS\Config\ConfigParserException; use DocbookCS\Config\SniffEntry; +use DocbookCS\Xml\XmlParser; use PHPUnit\Framework\Attributes\CoversClass; use PHPUnit\Framework\Attributes\Test; +use PHPUnit\Framework\Attributes\UsesClass; use PHPUnit\Framework\TestCase; #[ @@ -17,6 +19,8 @@ CoversClass(ConfigParser::class), CoversClass(ConfigParserException::class), CoversClass(SniffEntry::class), + // + UsesClass(XmlParser::class), ] final class ConfigParserTest extends TestCase { diff --git a/tests/Unit/Diff/DiffChangesetTest.php b/tests/Unit/Diff/DiffChangesetTest.php new file mode 100644 index 0000000..23fea73 --- /dev/null +++ b/tests/Unit/Diff/DiffChangesetTest.php @@ -0,0 +1,37 @@ +changeFor('/project/reference/file.xml')); + } + + #[Test] + public function itReturnsNullForUnchangedPaths(): void + { + $changeset = new DiffChangeset([new FileChange('reference/file.xml', [2])]); + + self::assertNull($changeset->changeFor('/project/reference/other.xml')); + } +} diff --git a/tests/Unit/Diff/UpstreamResolverTest.php b/tests/Unit/Diff/UpstreamResolverTest.php new file mode 100644 index 0000000..94bf9d4 --- /dev/null +++ b/tests/Unit/Diff/UpstreamResolverTest.php @@ -0,0 +1,116 @@ +workspace = sys_get_temp_dir() . '/docbook-cs-upstream-' . bin2hex(random_bytes(6)); + mkdir($this->workspace); + } + + protected function tearDown(): void + { + $files = new \RecursiveIteratorIterator( + new \RecursiveDirectoryIterator($this->workspace, \FilesystemIterator::SKIP_DOTS), + \RecursiveIteratorIterator::CHILD_FIRST, + ); + + foreach ($files as $file) { + if (!$file instanceof \SplFileInfo) { + throw new \UnexpectedValueException('Unexpected directory entry.'); + } + + $file->isDir() ? rmdir($file->getPathname()) : unlink($file->getPathname()); + } + + rmdir($this->workspace); + } + + #[Test] + public function itFallsBackWhenTheCacheDirectoryCannotBeCreated(): void + { + $cachePath = $this->workspace . '/cache'; + file_put_contents($cachePath, 'not a directory'); + + self::assertNull($this->resolver($this->successfulRunner(), $cachePath)->resolve('/repo', 'doc-en')); + } + + #[Test] + public function itFallsBackWhenTheCacheLockCannotBeOpened(): void + { + $cachePath = $this->workspace . '/cache'; + mkdir($cachePath); + mkdir($cachePath . '/doc-en.lock'); + + self::assertNull($this->resolver($this->successfulRunner(), $cachePath)->resolve('/repo', 'doc-en')); + } + + #[Test] + public function itFallsBackWhenTheCacheRepositoryCannotBeInitialised(): void + { + $runner = new class implements ProcessRunnerInterface { + public function run(array $command, string $workingDirectory, array $environment = []): ProcessResult + { + return new ProcessResult(1, '', 'Could not initialise cache.'); + } + }; + + self::assertNull($this->resolver($runner)->resolve('/repo', 'doc-en')); + } + + #[Test] + public function itFallsBackWhenGitCannotBeStarted(): void + { + $runner = new class implements ProcessRunnerInterface { + public function run(array $command, string $workingDirectory, array $environment = []): ProcessResult + { + throw ProcessException::couldNotStart(); + } + }; + + self::assertNull($this->resolver($runner)->resolve('/repo', 'doc-en')); + } + + private function resolver(ProcessRunnerInterface $runner, ?string $cachePath = null): UpstreamResolver + { + return new UpstreamResolver( + new GitClient($runner), + $cachePath ?? $this->workspace . '/cache', + ); + } + + private function successfulRunner(): ProcessRunnerInterface + { + return new class implements ProcessRunnerInterface { + public function run(array $command, string $workingDirectory, array $environment = []): ProcessResult + { + return new ProcessResult(0, '', ''); + } + }; + } +} diff --git a/tests/Unit/Fix/AttributeOrderFixerTest.php b/tests/Unit/Fix/AttributeOrderFixerTest.php index 562967e..9c65253 100644 --- a/tests/Unit/Fix/AttributeOrderFixerTest.php +++ b/tests/Unit/Fix/AttributeOrderFixerTest.php @@ -9,7 +9,6 @@ use DocbookCS\Fix\FixPlan; use DocbookCS\Fix\Fixer\AttributeOrderFixer; use DocbookCS\Fix\FixResult; -use DocbookCS\Runner\RunMode; use DocbookCS\Sniff\AttributeOrderSniff; use DocbookCS\Source\File; use DocbookCS\Source\Line; @@ -26,7 +25,6 @@ CoversClass(Fix::class), CoversClass(FixApplier::class), CoversClass(FixResult::class), - CoversClass(RunMode::class), CoversClass(Violation::class), // UsesClass(File::class), @@ -43,7 +41,8 @@ public function itMovesXmlIdBeforeXmlns(): void $document = $this->createDocument($content); $source = new File('file.xml', $content); - $violations = new AttributeOrderSniff(RunMode::Fix)->process($document, $source); + $sniffer = new AttributeOrderSniff(); + $violations = $sniffer->process($document, $source); $beginOffset = (int)strpos($content, 'rangeOne()->untilOffset); self::assertSame(1, $violations[0]->rangeOne()->line); - $fix = new AttributeOrderFixer()->process($violations[0]); + $fix = new ($sniffer::getFixerClassName())()->process($violations[0]); $result = new FixApplier()->apply($source, [ $fix ]); @@ -70,7 +69,7 @@ public function itPreservesTagShapedTextInsideComments(): void $document = $this->createDocument($content); $source = new File('file.xml', $content); - $violations = new AttributeOrderSniff(RunMode::Fix)->process($document, $source); + $violations = new AttributeOrderSniff()->process($document, $source); self::assertCount(1, $violations); diff --git a/tests/Unit/Fix/ExceptionNameFixerTest.php b/tests/Unit/Fix/ExceptionNameFixerTest.php index 7812eb1..832fd32 100644 --- a/tests/Unit/Fix/ExceptionNameFixerTest.php +++ b/tests/Unit/Fix/ExceptionNameFixerTest.php @@ -10,7 +10,6 @@ use DocbookCS\Fix\Fixer\ExceptionNameFixer; use DocbookCS\Fix\FixResult; use DocbookCS\Runner\EntityExpansionMarker; -use DocbookCS\Runner\RunMode; use DocbookCS\Sniff\ExceptionNameSniff; use DocbookCS\Source\File; use DocbookCS\Source\Line; @@ -27,7 +26,6 @@ CoversClass(Fix::class), CoversClass(FixApplier::class), CoversClass(FixResult::class), - CoversClass(RunMode::class), CoversClass(Violation::class), // UsesClass(EntityExpansionMarker::class), @@ -45,7 +43,8 @@ public function itReplacesSimpleClassnameTags(): void $document = $this->createDocument($content); $source = new File('file.xml', $content); - $violations = new ExceptionNameSniff(RunMode::Fix)->process($document, $source); + $sniffer = new ExceptionNameSniff(); + $violations = $sniffer->process($document, $source); $beginOffset = (int) strpos($content, ''); $untilOffset = (int) strpos($content, ''); @@ -58,7 +57,7 @@ public function itReplacesSimpleClassnameTags(): void new SourceRange(1, $untilOffset + 2, $untilOffset + 11, 'classname'), ], $violations[0]->affectedRanges); - $fix = new ExceptionNameFixer()->process($violations[0]); + $fix = new ($sniffer::getFixerClassName())()->process($violations[0]); $result = new FixApplier()->apply($source, [$fix]); @@ -73,7 +72,7 @@ public function itPreservesClassnameAttributes(): void $document = $this->createDocument($content); $source = new File('file.xml', $content); - $violations = new ExceptionNameSniff(RunMode::Fix)->process($document, $source); + $violations = new ExceptionNameSniff()->process($document, $source); self::assertCount(1, $violations); self::assertSame('classname', $violations[0]->rangeOne()->content); @@ -97,7 +96,7 @@ public function itKeepsSourceContentAlignedAfterRegularClassnames(): void $document = $this->createDocument($content); $source = new File('file.xml', $content); - $violations = new ExceptionNameSniff(RunMode::Fix)->process($document, $source); + $violations = new ExceptionNameSniff()->process($document, $source); $sourceContent = 'RuntimeException'; $beginOffset = (int) strpos($content, $sourceContent); @@ -130,7 +129,7 @@ public function itPreservesTagShapedTextInsideComments(): void $document = $this->createDocument($content); $source = new File('file.xml', $content); - $violations = new ExceptionNameSniff(RunMode::Fix)->process($document, $source); + $violations = new ExceptionNameSniff()->process($document, $source); self::assertCount(1, $violations); diff --git a/tests/Unit/Fix/FixPlanTest.php b/tests/Unit/Fix/FixPlanTest.php index 490938c..56c27a3 100644 --- a/tests/Unit/Fix/FixPlanTest.php +++ b/tests/Unit/Fix/FixPlanTest.php @@ -24,6 +24,73 @@ ] final class FixPlanTest extends TestCase { + #[Test] + public function itLeavesTheSourceUntouchedWithoutFixes(): void + { + $source = new File('file.xml', ''); + + $result = new FixApplier()->apply($source, []); + + self::assertSame($source, $result->file); + self::assertSame(0, $result->applied); + self::assertSame(0, $result->skipped); + } + + #[Test] + public function itSkipsFixesThatAlreadyHaveTheDesiredContent(): void + { + $source = new File('file.xml', ''); + $fix = new Fix('file.xml', 1, 5, 'root', 'Sniff', 'root'); + + $result = new FixApplier()->apply($source, [$fix]); + + self::assertSame($source->content, $result->file->content); + self::assertSame(0, $result->applied); + self::assertSame(1, $result->skipped); + } + + #[Test] + public function itSkipsCompetingInsertionsAtTheSameOffset(): void + { + $source = new File('file.xml', 'abc'); + $first = new Fix('file.xml', 1, 1, 'X', 'FirstSniff'); + $second = new Fix('file.xml', 1, 1, 'X', 'SecondSniff'); + + $result = new FixApplier()->apply($source, [$first, $second]); + + self::assertSame('aXbc', $result->file->content); + self::assertSame(1, $result->applied); + self::assertSame(1, $result->skipped); + } + + #[Test] + public function itAllowsAnInsertionAtTheStartOfAReplacement(): void + { + $source = new File('file.xml', 'abc'); + $insertion = new Fix('file.xml', 1, 1, 'X', 'InsertionSniff'); + $replacement = new Fix('file.xml', 1, 2, 'B', 'ReplacementSniff', 'b'); + + $result = new FixApplier()->apply($source, [$insertion, $replacement]); + + self::assertSame('aXBc', $result->file->content); + self::assertSame(2, $result->applied); + self::assertSame(0, $result->skipped); + } + + #[Test] + public function itSkipsAnInsertionInsideAReplacement(): void + { + $source = new File('file.xml', 'abc'); + $replacement = new Fix('file.xml', 0, 3, 'ABC', 'ReplacementSniff', 'abc'); + $insertion = new Fix('file.xml', 1, 1, 'X', 'InsertionSniff'); + + $result = new FixApplier()->apply($source, [$replacement, $insertion]); + + self::assertSame('ABC', $result->file->content); + self::assertSame(1, $result->applied); + self::assertSame(1, $result->skipped); + } + #[Test] public function itAppliesEveryFixInAPlanAtomically(): void { diff --git a/tests/Unit/Fix/FixerInputValidationTest.php b/tests/Unit/Fix/FixerInputValidationTest.php new file mode 100644 index 0000000..7163752 --- /dev/null +++ b/tests/Unit/Fix/FixerInputValidationTest.php @@ -0,0 +1,130 @@ + $ranges */ + #[Test, DataProvider('missingContent')] + public function itRejectsAffectedRangesWithoutSourceContent(Fixer $fixer, array $ranges): void + { + $this->expectException(FixerException::class); + $this->expectExceptionMessageIs('Fixers require affected source ranges with source content.'); + + $fixer->process($this->violation($ranges)); + } + + /** @return iterable}> */ + public static function missingContent(): iterable + { + yield 'attribute order' => [new AttributeOrderFixer(), [new SourceRange(1, 0, 4)]]; + yield 'exception name' => [new ExceptionNameFixer(), [ + new SourceRange(1, 0, 9), + new SourceRange(1, 10, 19), + ]]; + yield 'mixed indentation' => [new MixedIndentationFixer(), [new SourceRange(1, 0, 2)]]; + yield 'simpara' => [new SimparaFixer(), [ + new SourceRange(1, 0, 4), + new SourceRange(1, 5, 9), + ]]; + yield 'trailing whitespace' => [new TrailingWhitespaceFixer(), [new SourceRange(1, 0, 2)]]; + } + + /** @param non-empty-list $ranges */ + #[Test, DataProvider('invalidContent')] + public function itRejectsInvalidSourceContent(Fixer $fixer, array $ranges): void + { + $this->expectException(FixerException::class); + $m = 'Cannot fix violation T.Sniff in f.xml on line 1 because its source content is not valid fixer input.'; + $this->expectExceptionMessageIs($m); + + $fixer->process($this->violation($ranges)); + } + + /** @return iterable}> */ + public static function invalidContent(): iterable + { + yield 'attribute order' => [new AttributeOrderFixer(), [new SourceRange(1, 0, 6, '')]]; + yield 'exception name' => [new ExceptionNameFixer(), [ + new SourceRange(1, 0, 5, 'class'), + new SourceRange(1, 6, 11, 'class'), + ]]; + yield 'mixed indentation' => [new MixedIndentationFixer(), [new SourceRange(1, 0, 2, ' ')]]; + yield 'simpara' => [new SimparaFixer(), [ + new SourceRange(1, 0, 4, 'span'), + new SourceRange(1, 5, 9, 'span'), + ]]; + yield 'trailing whitespace' => [new TrailingWhitespaceFixer(), [new SourceRange(1, 0, 4, 'text')]]; + } + + #[Test, DataProvider('pairedElementFixers')] + public function itRejectsIncompletePairedElementRanges(Fixer $fixer): void + { + $this->expectException(FixerException::class); + + $fixer->process($this->violation([new SourceRange(1, 0, 4, 'para')])); + } + + /** @return iterable */ + public static function pairedElementFixers(): iterable + { + yield 'exception name' => [new ExceptionNameFixer()]; + yield 'simpara' => [new SimparaFixer()]; + } + + #[Test] + public function itRejectsMalformedAttributeOrderInput(): void + { + $this->expectException(FixerException::class); + + new AttributeOrderFixer()->process($this->violation([ + new SourceRange(1, 0, 9, 'not a tag'), + ])); + } + + #[Test] + public function itRejectsAttributeOrderInputThatIsAlreadyOrdered(): void + { + $content = ''; + + $this->expectException(FixerException::class); + + new AttributeOrderFixer()->process($this->violation([ + new SourceRange(1, 0, strlen($content), $content), + ])); + } + + /** @param non-empty-list $ranges */ + private function violation(array $ranges): Violation + { + return new Violation('T.Sniff', 'f.xml', 'violation.', $ranges); + } +} diff --git a/tests/Unit/Fix/MixedUnionFixerTest.php b/tests/Unit/Fix/MixedUnionFixerTest.php index 216fc00..1dde6e9 100644 --- a/tests/Unit/Fix/MixedUnionFixerTest.php +++ b/tests/Unit/Fix/MixedUnionFixerTest.php @@ -6,6 +6,7 @@ use DocbookCS\Fix\Fix; use DocbookCS\Fix\FixApplier; +use DocbookCS\Fix\FixPlan; use DocbookCS\Fix\Fixer\MixedUnionFixer; use DocbookCS\Fix\FixerException; use DocbookCS\Fix\FixResult; @@ -28,6 +29,8 @@ UsesClass(File::class), UsesClass(Fix::class), UsesClass(FixApplier::class), + UsesClass(FixPlan::class), + UsesClass(FixerException::class), UsesClass(FixResult::class), UsesClass(Line::class), UsesClass(RunMode::class), diff --git a/tests/Unit/Fix/NativeTypeFixerTest.php b/tests/Unit/Fix/NativeTypeFixerTest.php index 69cab11..0d066b7 100644 --- a/tests/Unit/Fix/NativeTypeFixerTest.php +++ b/tests/Unit/Fix/NativeTypeFixerTest.php @@ -6,6 +6,7 @@ use DocbookCS\Fix\Fix; use DocbookCS\Fix\FixApplier; +use DocbookCS\Fix\FixPlan; use DocbookCS\Fix\Fixer\NativeTypeFixer; use DocbookCS\Fix\FixerException; use DocbookCS\Fix\FixResult; @@ -27,6 +28,8 @@ UsesClass(File::class), UsesClass(Fix::class), UsesClass(FixApplier::class), + UsesClass(FixPlan::class), + UsesClass(FixerException::class), UsesClass(FixResult::class), UsesClass(Line::class), UsesClass(RunMode::class), diff --git a/tests/Unit/Fix/SimparaFixerTest.php b/tests/Unit/Fix/SimparaFixerTest.php index 7ebedb0..25f4266 100644 --- a/tests/Unit/Fix/SimparaFixerTest.php +++ b/tests/Unit/Fix/SimparaFixerTest.php @@ -10,7 +10,6 @@ use DocbookCS\Fix\Fixer\SimparaFixer; use DocbookCS\Fix\FixResult; use DocbookCS\Runner\EntityExpansionMarker; -use DocbookCS\Runner\RunMode; use DocbookCS\Sniff\SimparaSniff; use DocbookCS\Source\File; use DocbookCS\Source\Line; @@ -25,7 +24,6 @@ CoversClass(Fix::class), CoversClass(FixApplier::class), CoversClass(FixResult::class), - CoversClass(RunMode::class), CoversClass(SimparaFixer::class), CoversClass(SimparaSniff::class), CoversClass(Violation::class), @@ -45,13 +43,14 @@ public function itReplacesSimpleParaTags(): void $document = $this->createDocument($content); $source = new File('file.xml', $content); - $violations = new SimparaSniff(RunMode::Fix)->process($document, $source); + $sniffer = new SimparaSniff(); + $violations = $sniffer->process($document, $source); self::assertCount(1, $violations); self::assertSame('para', $violations[0]->rangeOne()->content); self::assertSame('para', $violations[0]->rangeTwo()->content); - $fix = new SimparaFixer()->process($violations[0]); + $fix = new ($sniffer::getFixerClassName())()->process($violations[0]); $result = new FixApplier()->apply($source, [$fix]); @@ -66,7 +65,7 @@ public function itPreservesParaAttributes(): void $document = $this->createDocument($content); $source = new File('file.xml', $content); - $violations = new SimparaSniff(RunMode::Fix)->process($document, $source); + $violations = new SimparaSniff()->process($document, $source); self::assertCount(1, $violations); self::assertSame('para', $violations[0]->rangeOne()->content); @@ -87,7 +86,7 @@ public function itCanFixNestedInnerParas(): void $document = $this->createDocument($content); $source = new File('file.xml', $content); - $violations = new SimparaSniff(RunMode::Fix)->process($document, $source); + $violations = new SimparaSniff()->process($document, $source); self::assertCount(1, $violations); self::assertSame('para', $violations[0]->rangeOne()->content); @@ -111,7 +110,7 @@ public function itPreservesTagShapedTextInsideComments(): void $document = $this->createDocument($content); $source = new File('file.xml', $content); - $violations = new SimparaSniff(RunMode::Fix)->process($document, $source); + $violations = new SimparaSniff()->process($document, $source); self::assertCount(1, $violations); diff --git a/tests/Unit/Fix/WhitespaceConcernFixersTest.php b/tests/Unit/Fix/WhitespaceConcernFixersTest.php index 1563773..5067538 100644 --- a/tests/Unit/Fix/WhitespaceConcernFixersTest.php +++ b/tests/Unit/Fix/WhitespaceConcernFixersTest.php @@ -10,7 +10,6 @@ use DocbookCS\Fix\Fixer\MixedIndentationFixer; use DocbookCS\Fix\Fixer\TrailingWhitespaceFixer; use DocbookCS\Fix\FixResult; -use DocbookCS\Runner\RunMode; use DocbookCS\Sniff\MixedIndentationSniff; use DocbookCS\Sniff\TrailingWhitespaceSniff; use DocbookCS\Source\File; @@ -47,19 +46,22 @@ public function itFixesIndependentWhitespaceConcernsTogether(): void $document->loadXML($content); $source = new File('file.xml', $content); - $trailingViolations = new TrailingWhitespaceSniff(RunMode::Fix)->process($document, $source); - $indentationViolations = new MixedIndentationSniff(RunMode::Fix)->process($document, $source); + $trailingSniffer = new TrailingWhitespaceSniff(); + $indentationSniffer = new MixedIndentationSniff(); + + $trailingViolations = $trailingSniffer->process($document, $source); + $indentationViolations = $indentationSniffer->process($document, $source); self::assertCount(2, $trailingViolations); self::assertCount(1, $indentationViolations); $fixes = []; - $trailingFixer = new TrailingWhitespaceFixer(); + $trailingFixer = new ($trailingSniffer::getFixerClassName())(); foreach ($trailingViolations as $violation) { $fixes[] = $trailingFixer->process($violation); } - $indentationFixer = new MixedIndentationFixer(); + $indentationFixer = new ($indentationSniffer::getFixerClassName())(); foreach ($indentationViolations as $violation) { $fixes[] = $indentationFixer->process($violation); } diff --git a/tests/Unit/Process/NativeProcessRunnerTest.php b/tests/Unit/Process/NativeProcessRunnerTest.php index 4b5eb06..81c954a 100644 --- a/tests/Unit/Process/NativeProcessRunnerTest.php +++ b/tests/Unit/Process/NativeProcessRunnerTest.php @@ -5,6 +5,7 @@ namespace DocbookCS\Tests\Unit\Process; use DocbookCS\Process\NativeProcessRunner; +use DocbookCS\Process\ProcessException; use DocbookCS\Process\ProcessResult; use PHPUnit\Framework\Attributes\CoversClass; use PHPUnit\Framework\Attributes\Test; @@ -12,6 +13,7 @@ #[ CoversClass(NativeProcessRunner::class), + CoversClass(ProcessException::class), CoversClass(ProcessResult::class), ] final class NativeProcessRunnerTest extends TestCase @@ -46,4 +48,16 @@ public function itPassesEnvironmentVariablesToTheProcess(): void self::assertSame(0, $result->exitCode); self::assertSame('value', $result->stdout); } + + #[Test] + public function itRejectsAnInvalidWorkingDirectory(): void + { + $this->expectException(ProcessException::class); + $this->expectExceptionMessageIs('Could not start process.'); + + new NativeProcessRunner()->run( + [PHP_BINARY, '-r', ''], + sys_get_temp_dir() . '/missing-' . bin2hex(random_bytes(6)), + ); + } } diff --git a/tests/Unit/Progress/ConsoleProgressTest.php b/tests/Unit/Progress/ConsoleProgressTest.php index b2ef99c..7f2ca2b 100644 --- a/tests/Unit/Progress/ConsoleProgressTest.php +++ b/tests/Unit/Progress/ConsoleProgressTest.php @@ -43,9 +43,19 @@ public function itDisplaysTotalFileCountOnStart(): void { $progress = new ConsoleProgress($this->stream, useColors: false); - $progress->start(42); + $progress->start(1_042); - self::assertStringContainsString('42 file(s)', $this->outputConsole()); + self::assertStringContainsString('1,042 files', $this->outputConsole()); + } + + #[Test] + public function itUsesTheSingularFileLabel(): void + { + $progress = new ConsoleProgress($this->stream, useColors: false); + + $progress->start(1); + + self::assertStringContainsString('1 file', $this->outputConsole()); } #[Test] diff --git a/tests/Unit/Report/ReportTest.php b/tests/Unit/Report/ReportTest.php index 4a50dac..4c5013b 100644 --- a/tests/Unit/Report/ReportTest.php +++ b/tests/Unit/Report/ReportTest.php @@ -7,6 +7,7 @@ use DocbookCS\RelativePath; use DocbookCS\Report\FileReport; use DocbookCS\Report\Report; +use DocbookCS\Report\ReportException; use DocbookCS\Violation\Severity; use DocbookCS\Violation\SourceRange; use DocbookCS\Violation\Violation; @@ -19,6 +20,7 @@ CoversClass(FileReport::class), CoversClass(RelativePath::class), CoversClass(Report::class), + CoversClass(ReportException::class), CoversClass(Violation::class), // UsesClass(SourceRange::class), @@ -27,12 +29,15 @@ final class ReportTest extends TestCase { private function createViolation( string $message = 'Some problem', - int $line = 1, - string $sniffCode = 'DocbookCS.Test', Severity $severity = Severity::ERROR, - string $filePath = 'file.xml', ): Violation { - return new Violation($sniffCode, $filePath, $message, [new SourceRange($line, 0, 0)], severity: $severity); + return new Violation( + 'DocbookCS.Test', + 'file.xml', + $message, + [new SourceRange(1, 0, 0)], + severity: $severity, + ); } #[Test] @@ -40,18 +45,18 @@ public function itStartsWithZeroFilesScanned(): void { $report = new Report(); - self::assertSame(0, $report->getFilesScanned()); + self::assertSame(0, $report->getScannedFilesCount()); } #[Test] - public function itIncrementsFilesScanned(): void + public function itCountsFileReportsAsScannedFiles(): void { $report = new Report(); - $report->incrementFilesScanned(); - $report->incrementFilesScanned(); - $report->incrementFilesScanned(); + $report->addFileReport(new FileReport('a.xml')); + $report->addFileReport(new FileReport('b.xml')); + $report->addFileReport(new FileReport('c.xml')); - self::assertSame(3, $report->getFilesScanned()); + self::assertSame(3, $report->getScannedFilesCount()); } #[Test] @@ -59,7 +64,39 @@ public function itStartsWithNoFileReports(): void { $report = new Report(); - self::assertSame([], $report->getFileReports()); + self::assertSame([], $report->fileReports); + } + + #[Test] + public function itRejectsAddingFoundViolationsTwice(): void + { + $fileReport = new FileReport('file.xml'); + $fileReport->addFoundViolations([]); + + $this->expectException(ReportException::class); + + $fileReport->addFoundViolations([]); + } + + #[Test] + public function itRejectsAddingASecondFileFailure(): void + { + $fileReport = new FileReport('file.xml'); + $fileReport->addFailedViolation($this->createViolation()); + + $this->expectException(ReportException::class); + + $fileReport->addFailedViolation($this->createViolation()); + } + + #[Test] + public function itRejectsAddingFinalViolationsBeforeFoundViolations(): void + { + $fileReport = new FileReport('file.xml'); + + $this->expectException(ReportException::class); + + $fileReport->addFinalViolations([]); } #[Test] @@ -70,8 +107,8 @@ public function itAddsFileReport(): void $report->addFileReport($fileReport); - self::assertCount(1, $report->getFileReports()); - self::assertSame($fileReport, $report->getFileReports()['src/chapter.xml']); + self::assertCount(1, $report->fileReports); + self::assertSame($fileReport, $report->fileReports['src/chapter.xml']); } #[Test] @@ -91,7 +128,7 @@ public function itKeysFileReportsByFilePath(): void $report->addFileReport(new FileReport('a.xml')); $report->addFileReport(new FileReport('b.xml')); - $keys = array_keys($report->getFileReports()); + $keys = array_keys($report->fileReports); self::assertSame(['a.xml', 'b.xml'], $keys); } @@ -106,25 +143,27 @@ public function itOverwritesFileReportWithSamePath(): void $report->addFileReport($first); $report->addFileReport($second); - self::assertCount(1, $report->getFileReports()); - self::assertSame($second, $report->getFileReports()['file.xml']); + self::assertCount(1, $report->fileReports); + self::assertSame($second, $report->fileReports['file.xml']); } #[Test] public function itReturnsTotalViolationsAcrossAllFiles(): void { $file1 = new FileReport('a.xml'); - $file1->addViolation($this->createViolation(severity: Severity::ERROR)); - $file1->addViolation($this->createViolation(severity: Severity::WARNING)); + $file1->addFoundViolations([ + $this->createViolation(), + $this->createViolation(severity: Severity::WARNING), + ]); $file2 = new FileReport('b.xml'); - $file2->addViolation($this->createViolation(severity: Severity::ERROR)); + $file2->addFoundViolations([$this->createViolation()]); $report = new Report(); $report->addFileReport($file1); $report->addFileReport($file2); - self::assertSame(3, $report->getTotalViolations()); + self::assertSame(3, $report->getTotalFinalViolationCount()); } #[Test] @@ -132,25 +171,29 @@ public function itReturnsZeroTotalViolationsWhenEmpty(): void { $report = new Report(); - self::assertSame(0, $report->getTotalViolations()); + self::assertSame(0, $report->getTotalFinalViolationCount()); } #[Test] public function itReturnsTotalErrorsAcrossAllFiles(): void { $file1 = new FileReport('a.xml'); - $file1->addViolation($this->createViolation(severity: Severity::ERROR)); - $file1->addViolation($this->createViolation(severity: Severity::WARNING)); + $file1->addFoundViolations([ + $this->createViolation(), + $this->createViolation(severity: Severity::WARNING), + ]); $file2 = new FileReport('b.xml'); - $file2->addViolation($this->createViolation(severity: Severity::ERROR)); - $file2->addViolation($this->createViolation(severity: Severity::ERROR)); + $file2->addFoundViolations([ + $this->createViolation(), + $this->createViolation(), + ]); $report = new Report(); $report->addFileReport($file1); $report->addFileReport($file2); - self::assertSame(3, $report->getTotalErrors()); + self::assertSame(3, $report->getTotalErrorLevelViolationCount()); } #[Test] @@ -158,25 +201,29 @@ public function itReturnsZeroTotalErrorsWhenEmpty(): void { $report = new Report(); - self::assertSame(0, $report->getTotalErrors()); + self::assertSame(0, $report->getTotalErrorLevelViolationCount()); } #[Test] public function itReturnsTotalWarningsAcrossAllFiles(): void { $file1 = new FileReport('a.xml'); - $file1->addViolation($this->createViolation(severity: Severity::WARNING)); - $file1->addViolation($this->createViolation(severity: Severity::ERROR)); + $file1->addFoundViolations([ + $this->createViolation(severity: Severity::WARNING), + $this->createViolation(severity: Severity::ERROR), + ]); $file2 = new FileReport('b.xml'); - $file2->addViolation($this->createViolation(severity: Severity::WARNING)); - $file2->addViolation($this->createViolation(severity: Severity::WARNING)); + $file2->addFoundViolations([ + $this->createViolation(severity: Severity::WARNING), + $this->createViolation(severity: Severity::WARNING), + ]); $report = new Report(); $report->addFileReport($file1); $report->addFileReport($file2); - self::assertSame(3, $report->getTotalWarnings()); + self::assertSame(3, $report->getTotalWarningLevelViolationCount()); } #[Test] @@ -184,36 +231,58 @@ public function itReturnsZeroTotalWarningsWhenEmpty(): void { $report = new Report(); - self::assertSame(0, $report->getTotalWarnings()); + self::assertSame(0, $report->getTotalWarningLevelViolationCount()); + } + + #[Test] + public function itReturnsTotalInfoViolationsAcrossAllFiles(): void + { + $file1 = new FileReport('a.xml'); + $file1->addFoundViolations([ + $this->createViolation(severity: Severity::INFO), + $this->createViolation(severity: Severity::ERROR), + ]); + + $file2 = new FileReport('b.xml'); + $file2->addFoundViolations([ + $this->createViolation(severity: Severity::INFO), + $this->createViolation(severity: Severity::INFO), + ]); + + $report = new Report(); + $report->addFileReport($file1); + $report->addFileReport($file2); + + self::assertSame(3, $report->getTotalInfoLevelViolationCount()); } #[Test] - public function itHasViolationsWhenViolationsExist(): void + public function itHasFinalViolationsWhenFinalViolationsExist(): void { $fileReport = new FileReport('file.xml'); - $fileReport->addViolation($this->createViolation()); + $fileReport->addFoundViolations([$this->createViolation()]); $report = new Report(); $report->addFileReport($fileReport); - self::assertTrue($report->hasViolations()); + self::assertTrue($report->hasFinalViolations()); } #[Test] - public function itHasNoViolationsWhenEmpty(): void + public function itHasNoFinalViolationsWhenEmpty(): void { $report = new Report(); - self::assertFalse($report->hasViolations()); + self::assertFalse($report->hasFinalViolations()); } #[Test] - public function itHasNoViolationsWhenFilesAreClean(): void + public function itHasNoFinalViolationsWhenFilesAreClean(): void { $report = new Report(); $report->addFileReport(new FileReport('clean.xml')); - self::assertFalse($report->hasViolations()); + self::assertFalse($report->hasFinalViolations()); } #[Test] @@ -224,15 +293,15 @@ public function itReturnsAllViolationsFromAllFiles(): void $v3 = $this->createViolation(message: 'Third'); $file1 = new FileReport('a.xml'); - $file1->addViolation($v1); - $file1->addViolation($v2); + $file1->addFoundViolations([$v1, $v2]); $file2 = new FileReport('b.xml'); - $file2->addViolation($v3); + $file2->addFoundViolations([$v3]); $report = new Report(); $report->addFileReport($file1); $report->addFileReport($file2); + $report->addFileReport(new FileReport('clean.xml')); $all = $report->getAllViolations(); @@ -254,41 +323,171 @@ public function itReturnsEmptyListWhenNoViolations(): void public function itDoesNotCountWarningsAsErrors(): void { $fileReport = new FileReport('file.xml'); - $fileReport->addViolation($this->createViolation(severity: Severity::WARNING)); - $fileReport->addViolation($this->createViolation(severity: Severity::WARNING)); + $fileReport->addFoundViolations([ + $this->createViolation(severity: Severity::WARNING), + $this->createViolation(severity: Severity::WARNING), + ]); $report = new Report(); $report->addFileReport($fileReport); - self::assertSame(0, $report->getTotalErrors()); - self::assertSame(2, $report->getTotalWarnings()); - self::assertSame(2, $report->getTotalViolations()); + self::assertSame(0, $report->getTotalErrorLevelViolationCount()); + self::assertSame(2, $report->getTotalWarningLevelViolationCount()); + self::assertSame(2, $report->getTotalFinalViolationCount()); } #[Test] public function itDoesNotCountErrorsAsWarnings(): void { $fileReport = new FileReport('file.xml'); - $fileReport->addViolation($this->createViolation(severity: Severity::ERROR)); - $fileReport->addViolation($this->createViolation(severity: Severity::ERROR)); + $fileReport->addFoundViolations([ + $this->createViolation(), + $this->createViolation(), + ]); $report = new Report(); $report->addFileReport($fileReport); - self::assertSame(2, $report->getTotalErrors()); - self::assertSame(0, $report->getTotalWarnings()); - self::assertSame(2, $report->getTotalViolations()); + self::assertSame(2, $report->getTotalErrorLevelViolationCount()); + self::assertSame(0, $report->getTotalWarningLevelViolationCount()); + self::assertSame(2, $report->getTotalFinalViolationCount()); } #[Test] - public function filesScannedIsIndependentOfFileReports(): void + public function itCountsCleanFileReportsAsScannedFiles(): void { $report = new Report(); - $report->incrementFilesScanned(); - $report->incrementFilesScanned(); - $report->incrementFilesScanned(); + $report->addFileReport(new FileReport('clean1.xml')); + $report->addFileReport(new FileReport('clean2.xml')); + $report->addFileReport(new FileReport('clean3.xml')); + + self::assertSame(3, $report->getScannedFilesCount()); + self::assertCount(3, $report->fileReports); + } + + #[Test] + public function itAggregatesFixingOutcome(): void + { + $first = new FileReport('first.xml'); + $first->markChanged(); + $first->recordFixingPass(); + $first->recordFixingPass(); + $first->addFoundViolations(array_fill(0, 4, $this->createViolation())); + $first->addFinalViolations([$this->createViolation()]); + + $second = new FileReport('second.xml'); + $second->markChanged(); + $second->recordFixingPass(); + $second->addFoundViolations(array_fill(0, 3, $this->createViolation())); + $second->addFinalViolations([$this->createViolation()]); + + $report = new Report(); + $report->addFileReport($first); + $report->addFileReport($second); + + self::assertSame(2, $report->getChangedFilesCount()); + self::assertSame(7, $report->getFoundViolationsCount()); + self::assertSame(5, $report->getAppliedFixesCount()); + self::assertSame(2, $report->getSkippedFixesCount()); + self::assertSame(5, $report->getFixedErrorCount()); + self::assertSame(0, $report->getFixedWarningCount()); + self::assertSame(3, $report->getFixingPassesCount()); + } + + #[Test] + public function itAggregatesSniffTimes(): void + { + $first = new FileReport('first.xml', collectPerformance: true); + $first->measureSniffer('Test.Sniff', static fn() => null); + + $second = new FileReport('second.xml', collectPerformance: true); + $second->measureSniffer('Test.Sniff', static fn() => null); + + $report = new Report(); + $report->addFileReport($first); + $report->addFileReport($second); + + self::assertSame( + $first->sniffingTimes['Test.Sniff'] + $second->sniffingTimes['Test.Sniff'], + $report->getSniffingTimes()['Test.Sniff'], + ); + } + + #[Test] + public function itAggregatesFixerTimes(): void + { + $first = new FileReport('first.xml', collectPerformance: true); + $first->measureFixer('Test.Sniff', static fn() => null); + + $second = new FileReport('second.xml', collectPerformance: true); + $second->measureFixer('Test.Sniff', static fn() => null); + + $report = new Report(); + $report->addFileReport($first); + $report->addFileReport($second); + + self::assertSame( + $first->fixingTimes['Test.Sniff'] + $second->fixingTimes['Test.Sniff'], + $report->getFixingTimes()['Test.Sniff'], + ); + } + + #[Test] + public function itMeasuresSniffingAndReturnsTheOperationResult(): void + { + $report = new Report(); + $fileReport = new FileReport('file.xml', collectPerformance: true); + $report->addFileReport($fileReport); + + $result = $fileReport->measureSniffing(static function (): string { + usleep(1_000); + + return 'result'; + }); + + self::assertSame('result', $result); + self::assertGreaterThan(0.0, $fileReport->totalSniffingTime); + self::assertSame($fileReport->totalSniffingTime, $report->getTotalSniffingTime()); + } + + #[Test] + public function itMeasuresFixingAndReturnsTheOperationResult(): void + { + $report = new Report(); + + + $fileReport = new FileReport('file.xml', collectPerformance: true); + $report->addFileReport($fileReport); + + $result = $fileReport->measureFixing(static function (): string { + usleep(1_000); + + return 'result'; + }); + + self::assertSame('result', $result); + self::assertGreaterThan(0.0, $report->getTotalFixingTime()); + } + + #[Test] + public function itRunsOperationsWithoutCollectingPerformanceByDefault(): void + { + $fileReport = new FileReport('file.xml'); + + $result = $fileReport->measureSniffer('Test.Sniff', static fn(): string => 'result'); + + self::assertSame('result', $result); + self::assertSame([], $fileReport->sniffingTimes); + } + + #[Test] + public function itRecordsFixingPasses(): void + { + $fileReport = new FileReport('file.xml'); + + $fileReport->recordFixingPass(); + $fileReport->recordFixingPass(); - self::assertSame(3, $report->getFilesScanned()); - self::assertCount(0, $report->getFileReports()); + self::assertSame(2, $fileReport->fixingPasses); } } diff --git a/tests/Unit/Report/Reporter/CheckstyleReporterTest.php b/tests/Unit/Report/Reporter/CheckstyleReporterTest.php index 69f2958..3291e36 100644 --- a/tests/Unit/Report/Reporter/CheckstyleReporterTest.php +++ b/tests/Unit/Report/Reporter/CheckstyleReporterTest.php @@ -92,6 +92,24 @@ public function itProducesNoFileNodesForEmptyReport(): void self::assertSame(0, $dom->getElementsByTagName('file')->length); } + #[Test] + public function itExcludesFixingOutcome(): void + { + $fileReport = new FileReport('fixed.xml'); + $fileReport->markChanged(); + $fileReport->recordFixingPass(); + + $report = new Report(); + $report->addFileReport($fileReport); + + $output = $this->reporter->generate($report); + $dom = $this->parseOutput($output); + + self::assertSame(0, $dom->getElementsByTagName('file')->length); + self::assertStringContainsString('total runtime:', $output); + self::assertStringNotContainsString('fix', $output); + } + #[Test] public function itSkipsFilesWithNoViolations(): void { @@ -107,7 +125,7 @@ public function itSkipsFilesWithNoViolations(): void public function itIncludesFileNodeWithNameAttribute(): void { $fileReport = new FileReport('src/broken.xml'); - $fileReport->addViolation($this->createViolation()); + $fileReport->addFoundViolations([$this->createViolation()]); $report = new Report(); $report->addFileReport($fileReport); @@ -123,7 +141,7 @@ public function itIncludesFileNodeWithNameAttribute(): void public function itRendersAbsoluteFilePathRelativeToWorkingDirectory(): void { $fileReport = new FileReport((getcwd() ?: '') . '/src/broken.xml'); - $fileReport->addViolation($this->createViolation()); + $fileReport->addFoundViolations([$this->createViolation()]); $report = new Report(); $report->addFileReport($fileReport); @@ -140,7 +158,7 @@ public function itRendersAbsoluteFilePathRelativeToWorkingDirectory(): void public function itSetsLineAttribute(): void { $fileReport = new FileReport('file.xml'); - $fileReport->addViolation($this->createViolation(line: 42)); + $fileReport->addFoundViolations([$this->createViolation(line: 42)]); $report = new Report(); $report->addFileReport($fileReport); @@ -155,7 +173,7 @@ public function itSetsLineAttribute(): void public function itSetsSeverityAttribute(): void { $fileReport = new FileReport('file.xml'); - $fileReport->addViolation($this->createViolation(severity: Severity::WARNING)); + $fileReport->addFoundViolations([$this->createViolation(severity: Severity::WARNING)]); $report = new Report(); $report->addFileReport($fileReport); @@ -170,7 +188,7 @@ public function itSetsSeverityAttribute(): void public function itSetsMessageAttribute(): void { $fileReport = new FileReport('file.xml'); - $fileReport->addViolation($this->createViolation(message: 'Use instead')); + $fileReport->addFoundViolations([$this->createViolation(message: 'Use instead')]); $report = new Report(); $report->addFileReport($fileReport); @@ -185,7 +203,7 @@ public function itSetsMessageAttribute(): void public function itSetsSourceAttribute(): void { $fileReport = new FileReport('file.xml'); - $fileReport->addViolation($this->createViolation(sniffCode: 'DocbookCS.ExceptionName')); + $fileReport->addFoundViolations([$this->createViolation(sniffCode: 'DocbookCS.ExceptionName')]); $report = new Report(); $report->addFileReport($fileReport); @@ -200,9 +218,11 @@ public function itSetsSourceAttribute(): void public function itOutputsMultipleViolationsForOneFile(): void { $fileReport = new FileReport('multi.xml'); - $fileReport->addViolation($this->createViolation(message: 'First', line: 5)); - $fileReport->addViolation($this->createViolation(message: 'Second', line: 10)); - $fileReport->addViolation($this->createViolation(message: 'Third', line: 20)); + $fileReport->addFoundViolations([ + $this->createViolation(message: 'First', line: 5), + $this->createViolation(message: 'Second', line: 10), + $this->createViolation(message: 'Third', line: 20), + ]); $report = new Report(); $report->addFileReport($fileReport); @@ -216,10 +236,10 @@ public function itOutputsMultipleViolationsForOneFile(): void public function itOutputsMultipleFilesWithViolations(): void { $file1 = new FileReport('first.xml'); - $file1->addViolation($this->createViolation(message: 'Issue A')); + $file1->addFoundViolations([$this->createViolation(message: 'Issue A')]); $file2 = new FileReport('second.xml'); - $file2->addViolation($this->createViolation(message: 'Issue B')); + $file2->addFoundViolations([$this->createViolation(message: 'Issue B')]); $report = new Report(); $report->addFileReport($file1); @@ -239,7 +259,7 @@ public function itSkipsCleanFilesAmongDirtyOnes(): void $cleanFile = new FileReport('clean.xml'); $dirtyFile = new FileReport('dirty.xml'); - $dirtyFile->addViolation($this->createViolation()); + $dirtyFile->addFoundViolations([$this->createViolation()]); $report = new Report(); $report->addFileReport($cleanFile); @@ -256,7 +276,7 @@ public function itSkipsCleanFilesAmongDirtyOnes(): void public function itEscapesSpecialCharactersInMessage(): void { $fileReport = new FileReport('escape.xml'); - $fileReport->addViolation($this->createViolation(message: 'Use "quotes" & ')); + $fileReport->addFoundViolations([$this->createViolation(message: 'Use "quotes" & ')]); $report = new Report(); $report->addFileReport($fileReport); diff --git a/tests/Unit/Report/Reporter/ConsoleReporterTest.php b/tests/Unit/Report/Reporter/ConsoleReporterTest.php index ad71756..453f4f1 100644 --- a/tests/Unit/Report/Reporter/ConsoleReporterTest.php +++ b/tests/Unit/Report/Reporter/ConsoleReporterTest.php @@ -39,9 +39,8 @@ private function createViolation( int $line = 1, string $sniffCode = 'DocbookCS.Test', Severity $severity = Severity::ERROR, - string $filePath = 'filepath.xml', ): Violation { - return new Violation($sniffCode, $filePath, $message, [new SourceRange($line, 0, 0)], severity: $severity); + return new Violation($sniffCode, 'filepath.xml', $message, [new SourceRange($line, 0, 0)], severity: $severity); } #[Test] @@ -59,34 +58,153 @@ public function itShowsOkSummaryWhenNoViolations(): void { $report = new Report(); $report->addFileReport(new FileReport('clean.xml')); - $report->incrementFilesScanned(); $output = $this->reporter->generate($report); - self::assertStringContainsString('OK -- 1 file(s) scanned, no violations found.', $output); + self::assertStringContainsString('OK -- 1 file scanned, no violations found.', $output); + self::assertStringNotContainsString('FIXING', $output); + } + + #[Test] + public function itShowsNoViolationsRemainingAfterFixing(): void + { + $report = new Report(); + + $fileReport = new FileReport('fixed.xml'); + $violation = $this->createViolation(); + $fileReport->addFoundViolations([$violation]); + $fileReport->addFinalViolations([]); + $fileReport->markChanged(); + $fileReport->recordFixingPass(); + + $report->addFileReport($fileReport); + + $output = $this->reporter->generate($report); + + self::assertStringContainsString( + 'FIXED 1 violation [1 error, 0 warnings] in 1 file, no violations remaining.', + $output, + ); + self::assertStringNotContainsString('REMAINING', $output); + self::assertStringNotContainsString('OK --', $output); + self::assertStringNotContainsString('passes)', $output); + } + + #[Test] + public function itShowsViolationsRemainingAfterFixing(): void + { + $fileReport = new FileReport('dirty.xml'); + $violation = $this->createViolation(); + $fileReport->addFoundViolations([$violation]); + $fileReport->recordFixingPass(); + + $report = new Report(); + $report->addFileReport($fileReport); + + $output = $this->reporter->generate($report); + + self::assertStringContainsString('REMAINING 1 violation [1 error, 0 warnings] in 1 file.', $output); + self::assertStringNotContainsString('passes)', $output); } #[Test] public function itShowsViolationSummaryWhenViolationsExist(): void { $fileReport = new FileReport('dirty.xml'); - $fileReport->addViolation($this->createViolation(severity: Severity::ERROR)); - $fileReport->addViolation($this->createViolation(severity: Severity::WARNING)); + $fileReport->addFoundViolations([ + $this->createViolation(), + $this->createViolation(severity: Severity::WARNING), + ]); + + $report = new Report(); + $report->addFileReport($fileReport); + + $output = $this->reporter->generate($report); + + self::assertStringContainsString('FOUND 2 violations [1 error, 1 warning] in 1 file.', $output); + } + + #[Test] + public function itShowsRemainingViolationsAfterFixing(): void + { + $fileReport = new FileReport('dirty.xml'); + $violation = $this->createViolation(); + $fileReport->addFoundViolations([$violation]); + $fileReport->recordFixingPass(); + + $report = new Report(); + $report->addFileReport($fileReport); + + $output = $this->reporter->generate($report); + + self::assertStringContainsString( + 'REMAINING 1 violation [1 error, 0 warnings] in 1 file.', + $output, + ); + } + + #[Test] + public function itShowsFixedViolationsAndAdditionalPasses(): void + { + $report = new Report(); + + $first = new FileReport('first.xml'); + $first->markChanged(); + $first->recordFixingPass(); + $first->recordFixingPass(); + $first->addFoundViolations([ + ...array_fill(0, 3, $this->createViolation()), + $this->createViolation(severity: Severity::WARNING), + ]); + $first->addFinalViolations([$this->createViolation(severity: Severity::WARNING)]); + + $second = new FileReport('second.xml'); + $second->markChanged(); + $second->recordFixingPass(); + $second->addFoundViolations([ + $this->createViolation(), + $this->createViolation(severity: Severity::WARNING), + $this->createViolation(severity: Severity::WARNING), + ]); + $second->addFinalViolations([$this->createViolation()]); + + $report->addFileReport($first); + $report->addFileReport($second); + + $output = $this->reporter->generate($report); + + self::assertStringContainsString( + 'FIXED 5 violations [3 errors, 2 warnings] in 2 files (3 passes).', + $output, + ); + self::assertStringNotContainsString('FIXING', $output); + } + + #[Test] + public function itFormatsLargeSummaryCounts(): void + { + $fileReport = new FileReport('fixed.xml'); + $fileReport->markChanged(); + $fileReport->recordFixingPass(); + $fileReport->addFoundViolations(array_fill(0, 1_001, $this->createViolation())); + $fileReport->addFinalViolations([$this->createViolation()]); $report = new Report(); $report->addFileReport($fileReport); - $report->incrementFilesScanned(); $output = $this->reporter->generate($report); - self::assertStringContainsString('FOUND 2 violation(s) (1 error(s), 1 warning(s)) in 1 file(s).', $output); + self::assertStringContainsString( + 'FIXED 1,000 violations [1,000 errors, 0 warnings] in 1 file.', + $output, + ); } #[Test] public function itShowsFilePathInHeader(): void { $fileReport = new FileReport('src/broken.xml'); - $fileReport->addViolation($this->createViolation()); + $fileReport->addFoundViolations([$this->createViolation()]); $report = new Report(); $report->addFileReport($fileReport); @@ -100,7 +218,7 @@ public function itShowsFilePathInHeader(): void public function itRendersAbsoluteFilePathRelativeToWorkingDirectory(): void { $fileReport = new FileReport((getcwd() ?: '') . '/src/broken.xml'); - $fileReport->addViolation($this->createViolation()); + $fileReport->addFoundViolations([$this->createViolation()]); $report = new Report(); $report->addFileReport($fileReport); @@ -114,7 +232,7 @@ public function itRendersAbsoluteFilePathRelativeToWorkingDirectory(): void public function itShowsDashSeparatorAfterFileHeader(): void { $fileReport = new FileReport('file.xml'); - $fileReport->addViolation($this->createViolation()); + $fileReport->addFoundViolations([$this->createViolation()]); $report = new Report(); $report->addFileReport($fileReport); @@ -130,7 +248,7 @@ public function itCapsTheDashSeparatorAt80Characters(): void { $longPath = str_repeat('a', 200) . '.xml'; $fileReport = new FileReport($longPath); - $fileReport->addViolation($this->createViolation()); + $fileReport->addFoundViolations([$this->createViolation()]); $report = new Report(); $report->addFileReport($fileReport); @@ -145,7 +263,7 @@ public function itCapsTheDashSeparatorAt80Characters(): void public function itShowsLineNumberInViolation(): void { $fileReport = new FileReport('file.xml'); - $fileReport->addViolation($this->createViolation(line: 42)); + $fileReport->addFoundViolations([$this->createViolation(line: 42)]); $report = new Report(); $report->addFileReport($fileReport); @@ -159,7 +277,7 @@ public function itShowsLineNumberInViolation(): void public function itRightAlignsLineNumberIn4CharWidth(): void { $fileReport = new FileReport('file.xml'); - $fileReport->addViolation($this->createViolation(line: 5)); + $fileReport->addFoundViolations([$this->createViolation(line: 5)]); $report = new Report(); $report->addFileReport($fileReport); @@ -173,7 +291,7 @@ public function itRightAlignsLineNumberIn4CharWidth(): void public function itShowsMessageInViolation(): void { $fileReport = new FileReport('file.xml'); - $fileReport->addViolation($this->createViolation(message: 'Use instead')); + $fileReport->addFoundViolations([$this->createViolation(message: 'Use instead')]); $report = new Report(); $report->addFileReport($fileReport); @@ -187,7 +305,7 @@ public function itShowsMessageInViolation(): void public function itShowsSniffCodeInViolation(): void { $fileReport = new FileReport('file.xml'); - $fileReport->addViolation($this->createViolation(sniffCode: 'DocbookCS.ExceptionName')); + $fileReport->addFoundViolations([$this->createViolation(sniffCode: 'DocbookCS.ExceptionName')]); $report = new Report(); $report->addFileReport($fileReport); @@ -201,7 +319,7 @@ public function itShowsSniffCodeInViolation(): void public function itShowsErrorSeverityLabel(): void { $fileReport = new FileReport('file.xml'); - $fileReport->addViolation($this->createViolation(severity: Severity::ERROR)); + $fileReport->addFoundViolations([$this->createViolation()]); $report = new Report(); $report->addFileReport($fileReport); @@ -211,13 +329,27 @@ public function itShowsErrorSeverityLabel(): void self::assertStringContainsString('ERROR', $output); } + #[Test] + public function itShowsInfoSeverityLabel(): void + { + $fileReport = new FileReport('file.xml'); + $fileReport->addFoundViolations([$this->createViolation(severity: Severity::INFO)]); + + $report = new Report(); + $report->addFileReport($fileReport); + + self::assertStringContainsString('INFO', $this->reporter->generate($report)); + } + #[Test] public function itShowsMultipleViolationsForOneFile(): void { $fileReport = new FileReport('multi.xml'); - $fileReport->addViolation($this->createViolation(message: 'First issue', line: 5)); - $fileReport->addViolation($this->createViolation(message: 'Second issue', line: 10)); - $fileReport->addViolation($this->createViolation(message: 'Third issue', line: 20)); + $fileReport->addFoundViolations([ + $this->createViolation(message: 'First issue', line: 5), + $this->createViolation(message: 'Second issue', line: 10), + $this->createViolation(message: 'Third issue', line: 20), + ]); $report = new Report(); $report->addFileReport($fileReport); @@ -233,10 +365,10 @@ public function itShowsMultipleViolationsForOneFile(): void public function itShowsMultipleFileHeaders(): void { $file1 = new FileReport('first.xml'); - $file1->addViolation($this->createViolation(message: 'Issue A')); + $file1->addFoundViolations([$this->createViolation(message: 'Issue A')]); $file2 = new FileReport('second.xml'); - $file2->addViolation($this->createViolation(message: 'Issue B')); + $file2->addFoundViolations([$this->createViolation(message: 'Issue B')]); $report = new Report(); $report->addFileReport($file1); @@ -254,7 +386,7 @@ public function itSkipsCleanFilesAmongDirtyOnes(): void $cleanFile = new FileReport('clean.xml'); $dirtyFile = new FileReport('dirty.xml'); - $dirtyFile->addViolation($this->createViolation()); + $dirtyFile->addFoundViolations([$this->createViolation()]); $report = new Report(); $report->addFileReport($cleanFile); @@ -271,25 +403,22 @@ public function itShowsScannedFileCountInOkSummary(): void { $report = new Report(); $report->addFileReport(new FileReport('a.xml')); - $report->incrementFilesScanned(); $report->addFileReport(new FileReport('b.xml')); - $report->incrementFilesScanned(); $report->addFileReport(new FileReport('c.xml')); - $report->incrementFilesScanned(); $output = $this->reporter->generate($report); - self::assertStringContainsString('3 file(s) scanned', $output); + self::assertStringContainsString('3 files scanned', $output); } #[Test] public function itCountsFilesWithViolationsInFoundSummary(): void { $file1 = new FileReport('a.xml'); - $file1->addViolation($this->createViolation()); + $file1->addFoundViolations([$this->createViolation()]); $file2 = new FileReport('b.xml'); - $file2->addViolation($this->createViolation()); + $file2->addFoundViolations([$this->createViolation()]); $cleanFile = new FileReport('c.xml'); @@ -300,7 +429,7 @@ public function itCountsFilesWithViolationsInFoundSummary(): void $output = $this->reporter->generate($report); - self::assertStringContainsString('in 3 file(s).', $output); + self::assertStringContainsString('in 2 files.', $output); } #[Test] @@ -309,7 +438,7 @@ public function itAppliesAnsiCodesWhenColorsEnabled(): void $reporter = new ConsoleReporter(useColors: true); $report = new Report(); - $report->incrementFilesScanned(); + $report->addFileReport(new FileReport('clean.xml')); $output = $reporter->generate($report); @@ -320,7 +449,7 @@ public function itAppliesAnsiCodesWhenColorsEnabled(): void public function itOmitsAnsiCodesWhenColorsDisabled(): void { $report = new Report(); - $report->incrementFilesScanned(); + $report->addFileReport(new FileReport('clean.xml')); $output = $this->reporter->generate($report); @@ -333,7 +462,7 @@ public function itUsesColorsEnabledByDefault(): void $reporter = new ConsoleReporter(); $report = new Report(); - $report->incrementFilesScanned(); + $report->addFileReport(new FileReport('clean.xml')); $output = $reporter->generate($report); @@ -344,7 +473,7 @@ public function itUsesColorsEnabledByDefault(): void public function itPadsSeverityToSevenCharacters(): void { $fileReport = new FileReport('file.xml'); - $fileReport->addViolation($this->createViolation(severity: Severity::ERROR)); + $fileReport->addFoundViolations([$this->createViolation()]); $report = new Report(); $report->addFileReport($fileReport); @@ -358,12 +487,7 @@ public function itPadsSeverityToSevenCharacters(): void public function itSeparatesFieldsWithPipes(): void { $fileReport = new FileReport('file.xml'); - $fileReport->addViolation($this->createViolation( - message: 'Test message', - line: 1, - sniffCode: 'DocbookCS.Test', - severity: Severity::ERROR, - )); + $fileReport->addFoundViolations([$this->createViolation(message: 'Test message')]); $report = new Report(); $report->addFileReport($fileReport); @@ -389,7 +513,7 @@ public function itShowsNoPerformanceDataWhenEmpty(): void $reporter = new ConsoleReporter(useColors: false, showPerformance: true); $report = new Report(); - $report->incrementFilesScanned(); + $report->addFileReport(new FileReport('clean.xml')); $output = $reporter->generate($report); @@ -402,15 +526,17 @@ public function itShowsPerformanceSectionWithHeader(): void $reporter = new ConsoleReporter(useColors: false, showPerformance: true); $report = new Report(); - $report->incrementFilesScanned(); - - $report->setTotalTime(2.0); - $report->addSniffTime('SniffA', 1.0); + $report->measureWallTime(function () use ($report): void { + $fileReport = new FileReport('clean.xml', collectPerformance: true); + $fileReport->measureSniffer('SniffA', static fn() => usleep(1_000)); + $report->addFileReport($fileReport); + }); $output = $reporter->generate($report); self::assertStringContainsString('PERFORMANCE', $output); - self::assertStringContainsString('Total runtime: 2.000s', $output); + self::assertSame(1, substr_count($output, 'Total runtime:')); + self::assertStringContainsString('Sniffing', $output); } #[Test] @@ -419,11 +545,13 @@ public function itSortsSniffTimesBySlowestFirst(): void $reporter = new ConsoleReporter(useColors: false, showPerformance: true); $report = new Report(); - $report->setTotalTime(3.0); - - $report->addSniffTime('FastSniff', 0.5); - $report->addSniffTime('SlowSniff', 2.0); - $report->addSniffTime('MediumSniff', 1.0); + $report->measureWallTime(function () use ($report): void { + $fileReport = new FileReport('file.xml', collectPerformance: true); + $fileReport->measureSniffer('FastSniff', static fn() => usleep(1_000)); + $fileReport->measureSniffer('SlowSniff', static fn() => usleep(30_000)); + $fileReport->measureSniffer('MediumSniff', static fn() => usleep(10_000)); + $report->addFileReport($fileReport); + }); $output = $reporter->generate($report); @@ -441,13 +569,42 @@ public function itDisplaysTimeAndPercentagePerSniff(): void $reporter = new ConsoleReporter(useColors: false, showPerformance: true); $report = new Report(); - $report->setTotalTime(2.0); + $report->measureWallTime(function () use ($report): void { + $fileReport = new FileReport('file.xml', collectPerformance: true); + $fileReport->measureSniffer('SniffA', static fn() => usleep(1_000)); + $fileReport->measureSniffer('SniffB', static fn() => usleep(1_000)); + $report->addFileReport($fileReport); + }); + + $output = $reporter->generate($report); + + self::assertMatchesRegularExpression( + '/^ SniffA +\d+\.\d{3}s \( *\d+\.\d%\) *$/m', + $output, + ); + } + + #[Test] + public function itDisplaysFixingTimeAndPercentage(): void + { + $reporter = new ConsoleReporter(useColors: false, showPerformance: true); - $report->addSniffTime('SniffA', 1.0); // 50% + $report = new Report(); + $report->measureWallTime(function () use ($report): void { + $fileReport = new FileReport('file.xml', collectPerformance: true); + $fileReport->measureSniffer('SniffA', static fn() => usleep(1_000)); + $fileReport->measureFixing( + fn() => $fileReport->measureFixer('SniffA', static fn() => usleep(1_000)) + ); + $report->addFileReport($fileReport); + }); $output = $reporter->generate($report); - self::assertStringContainsString('1.000s ( 50.0%)', $output); + self::assertMatchesRegularExpression( + '/^ SniffA +\d+\.\d{3}s \( *\d+\.\d%\) +\d+\.\d{3}s \( *\d+\.\d%\) *$/m', + $output, + ); } #[Test] @@ -456,8 +613,11 @@ public function itDoesNotShowPerformanceWhenDisabled(): void $reporter = new ConsoleReporter(useColors: false, showPerformance: false); $report = new Report(); - $report->setTotalTime(2.0); - $report->addSniffTime('SniffA', 1.0); + + $fileReport = new FileReport('file.xml'); + $fileReport->measureSniffer('SniffA', static fn() => null); + + $report->addFileReport($fileReport); $output = $reporter->generate($report); diff --git a/tests/Unit/Report/Reporter/JsonReporterTest.php b/tests/Unit/Report/Reporter/JsonReporterTest.php index 8354a4c..83310e5 100644 --- a/tests/Unit/Report/Reporter/JsonReporterTest.php +++ b/tests/Unit/Report/Reporter/JsonReporterTest.php @@ -91,16 +91,44 @@ public function itCountsScannedFiles(): void { $report = new Report(); $report->addFileReport(new FileReport('a.xml')); - $report->incrementFilesScanned(); $report->addFileReport(new FileReport('b.xml')); - $report->incrementFilesScanned(); $data = $this->parseOutput($this->reporter->generate($report)); self::assertSame(2, $data['totals']['files_scanned'] ?? null); } + #[Test] + public function itIncludesFixingOutcome(): void + { + $first = new FileReport('first.xml'); + $first->markChanged(); + $first->recordFixingPass(); + $first->recordFixingPass(); + $first->addFoundViolations(array_fill(0, 4, $this->createViolation())); + $first->addFinalViolations([$this->createViolation()]); + + $second = new FileReport('second.xml'); + $second->markChanged(); + $second->recordFixingPass(); + $second->addFoundViolations(array_fill(0, 3, $this->createViolation())); + $second->addFinalViolations([$this->createViolation()]); + + $report = new Report(); + $report->addFileReport($first); + $report->addFileReport($second); + + $data = $this->parseOutput($this->reporter->generate($report)); + + self::assertSame([ + 'files_changed' => 2, + 'fixes_applied' => 5, + 'fixes_skipped' => 2, + 'fixing_passes' => 3, + ], $data['fixing']); + } + #[Test] public function itSkipsFilesWithNoViolations(): void { @@ -116,7 +144,7 @@ public function itSkipsFilesWithNoViolations(): void public function itIncludesFileWithViolations(): void { $fileReport = new FileReport('dirty.xml'); - $fileReport->addViolation($this->createViolation()); + $fileReport->addFoundViolations([$this->createViolation()]); $report = new Report(); $report->addFileReport($fileReport); @@ -130,8 +158,10 @@ public function itIncludesFileWithViolations(): void public function itSetsViolationCountPerFile(): void { $fileReport = new FileReport('file.xml'); - $fileReport->addViolation($this->createViolation(message: 'First')); - $fileReport->addViolation($this->createViolation(message: 'Second')); + $fileReport->addFoundViolations([ + $this->createViolation(message: 'First'), + $this->createViolation(message: 'Second'), + ]); $report = new Report(); $report->addFileReport($fileReport); @@ -145,7 +175,7 @@ public function itSetsViolationCountPerFile(): void public function itSetsLineInMessage(): void { $fileReport = new FileReport('file.xml'); - $fileReport->addViolation($this->createViolation(line: 42)); + $fileReport->addFoundViolations([$this->createViolation(line: 42)]); $report = new Report(); $report->addFileReport($fileReport); @@ -159,7 +189,7 @@ public function itSetsLineInMessage(): void public function itSetsSeverityInMessage(): void { $fileReport = new FileReport('file.xml'); - $fileReport->addViolation($this->createViolation(severity: Severity::WARNING)); + $fileReport->addFoundViolations([$this->createViolation(severity: Severity::WARNING)]); $report = new Report(); $report->addFileReport($fileReport); @@ -173,7 +203,7 @@ public function itSetsSeverityInMessage(): void public function itSetsMessageInMessage(): void { $fileReport = new FileReport('file.xml'); - $fileReport->addViolation($this->createViolation(message: 'Use instead')); + $fileReport->addFoundViolations([$this->createViolation(message: 'Use instead')]); $report = new Report(); $report->addFileReport($fileReport); @@ -187,7 +217,7 @@ public function itSetsMessageInMessage(): void public function itSetsSourceInMessage(): void { $fileReport = new FileReport('file.xml'); - $fileReport->addViolation($this->createViolation(sniffCode: 'DocbookCS.ExceptionName')); + $fileReport->addFoundViolations([$this->createViolation(sniffCode: 'DocbookCS.ExceptionName')]); $report = new Report(); $report->addFileReport($fileReport); @@ -201,9 +231,11 @@ public function itSetsSourceInMessage(): void public function itOutputsMultipleViolationsForOneFile(): void { $fileReport = new FileReport('multi.xml'); - $fileReport->addViolation($this->createViolation(message: 'First', line: 5)); - $fileReport->addViolation($this->createViolation(message: 'Second', line: 10)); - $fileReport->addViolation($this->createViolation(message: 'Third', line: 20)); + $fileReport->addFoundViolations([ + $this->createViolation(message: 'First', line: 5), + $this->createViolation(message: 'Second', line: 10), + $this->createViolation(message: 'Third', line: 20), + ]); $report = new Report(); $report->addFileReport($fileReport); @@ -217,10 +249,10 @@ public function itOutputsMultipleViolationsForOneFile(): void public function itOutputsMultipleFilesWithViolations(): void { $file1 = new FileReport('first.xml'); - $file1->addViolation($this->createViolation(message: 'Issue A')); + $file1->addFoundViolations([$this->createViolation(message: 'Issue A')]); $file2 = new FileReport('second.xml'); - $file2->addViolation($this->createViolation(message: 'Issue B')); + $file2->addFoundViolations([$this->createViolation(message: 'Issue B')]); $report = new Report(); $report->addFileReport($file1); @@ -239,7 +271,7 @@ public function itSkipsCleanFilesAmongDirtyOnes(): void $cleanFile = new FileReport('clean.xml'); $dirtyFile = new FileReport('dirty.xml'); - $dirtyFile->addViolation($this->createViolation()); + $dirtyFile->addFoundViolations([$this->createViolation()]); $report = new Report(); $report->addFileReport($cleanFile); @@ -256,11 +288,13 @@ public function itSkipsCleanFilesAmongDirtyOnes(): void public function itCountsTotalViolations(): void { $file1 = new FileReport('a.xml'); - $file1->addViolation($this->createViolation()); - $file1->addViolation($this->createViolation()); + $file1->addFoundViolations([ + $this->createViolation(), + $this->createViolation(), + ]); $file2 = new FileReport('b.xml'); - $file2->addViolation($this->createViolation()); + $file2->addFoundViolations([$this->createViolation()]); $report = new Report(); $report->addFileReport($file1); @@ -275,9 +309,11 @@ public function itCountsTotalViolations(): void public function itCountsTotalErrors(): void { $fileReport = new FileReport('file.xml'); - $fileReport->addViolation($this->createViolation(severity: Severity::ERROR)); - $fileReport->addViolation($this->createViolation(severity: Severity::WARNING)); - $fileReport->addViolation($this->createViolation(severity: Severity::ERROR)); + $fileReport->addFoundViolations([ + $this->createViolation(), + $this->createViolation(severity: Severity::WARNING), + $this->createViolation(), + ]); $report = new Report(); $report->addFileReport($fileReport); @@ -291,9 +327,11 @@ public function itCountsTotalErrors(): void public function itCountsTotalWarnings(): void { $fileReport = new FileReport('file.xml'); - $fileReport->addViolation($this->createViolation(severity: Severity::WARNING)); - $fileReport->addViolation($this->createViolation(severity: Severity::ERROR)); - $fileReport->addViolation($this->createViolation(severity: Severity::WARNING)); + $fileReport->addFoundViolations([ + $this->createViolation(severity: Severity::WARNING), + $this->createViolation(), + $this->createViolation(severity: Severity::WARNING), + ]); $report = new Report(); $report->addFileReport($fileReport); @@ -307,7 +345,7 @@ public function itCountsTotalWarnings(): void public function itDoesNotEscapeSlashesInOutput(): void { $fileReport = new FileReport('path/to/file.xml'); - $fileReport->addViolation($this->createViolation()); + $fileReport->addFoundViolations([$this->createViolation()]); $report = new Report(); $report->addFileReport($fileReport); @@ -322,7 +360,7 @@ public function itDoesNotEscapeSlashesInOutput(): void public function itRendersAbsoluteFilePathRelativeToWorkingDirectory(): void { $fileReport = new FileReport((getcwd() ?: '') . '/path/to/file.xml'); - $fileReport->addViolation($this->createViolation()); + $fileReport->addFoundViolations([$this->createViolation()]); $report = new Report(); $report->addFileReport($fileReport); @@ -358,7 +396,13 @@ public function itUsesPrettyPrintedJson(): void * message: string, * source: string * }> - * }> + * }>, + * fixing: array{ + * files_changed: int, + * fixes_applied: int, + * fixes_skipped: int, + * fixing_passes: int + * } * } */ private function parseOutput(string $json): array diff --git a/tests/Unit/Runner/FixConvergenceTest.php b/tests/Unit/Runner/FixConvergenceTest.php index 00cdf6d..f98d00c 100644 --- a/tests/Unit/Runner/FixConvergenceTest.php +++ b/tests/Unit/Runner/FixConvergenceTest.php @@ -16,11 +16,12 @@ use DocbookCS\Report\Report; use DocbookCS\Runner\EntityExpansionMarker; use DocbookCS\Runner\EntityPreprocessor; +use DocbookCS\Runner\XmlFileProcessor; +use DocbookCS\Runner\XmlFixRunner; use DocbookCS\Runner\RunMode; use DocbookCS\Runner\RunScope; use DocbookCS\Runner\ViolationScopeFilter; -use DocbookCS\Runner\XmlFileProcessor; -use DocbookCS\Runner\XmlProcessingResult; +use DocbookCS\Runner\XmlSniffRunner; use DocbookCS\Sniff\AbstractSniff; use DocbookCS\Sniff\ExceptionNameSniff; use DocbookCS\Sniff\Fixable; @@ -32,13 +33,17 @@ use DocbookCS\Tests\Support\Fix\ToggleElementFixer; use DocbookCS\Violation\SourceRange; use DocbookCS\Violation\Violation; +use DocbookCS\Xml\XmlParser; use PHPUnit\Framework\Attributes\CoversClass; use PHPUnit\Framework\Attributes\Test; use PHPUnit\Framework\Attributes\UsesClass; use PHPUnit\Framework\TestCase; #[ + CoversClass(FixerException::class), CoversClass(XmlFileProcessor::class), + CoversClass(XmlFixRunner::class), + CoversClass(XmlSniffRunner::class), // UsesClass(AbstractSniff::class), UsesClass(EntityExpansionMarker::class), @@ -50,19 +55,18 @@ UsesClass(FileReport::class), UsesClass(Fix::class), UsesClass(FixApplier::class), - UsesClass(FixerException::class), UsesClass(FixPlan::class), UsesClass(FixResult::class), UsesClass(Line::class), UsesClass(Report::class), UsesClass(RunMode::class), + UsesClass(RunScope::class), UsesClass(SimparaFixer::class), UsesClass(SimparaSniff::class), UsesClass(SourceRange::class), - UsesClass(RunScope::class), UsesClass(Violation::class), UsesClass(ViolationScopeFilter::class), - UsesClass(XmlProcessingResult::class), + UsesClass(XmlParser::class), ] final class FixConvergenceTest extends TestCase { @@ -73,10 +77,10 @@ public function itAppliesIndependentSameLineFixesAndReportsTheFinalSource(): voi $filePath = $this->temporaryFile($source); try { - $processor = new XmlFileProcessor([ - new SimparaSniff(RunMode::Fix), - new ExceptionNameSniff(RunMode::Fix), - ]); + $processor = new XmlFileProcessor(new XmlSniffRunner(RunMode::Fix, [ + new SimparaSniff(), + new ExceptionNameSniff(), + ])); $report = $this->processFile($processor, $filePath); @@ -84,7 +88,8 @@ public function itAppliesIndependentSameLineFixesAndReportsTheFinalSource(): voi 'ARuntimeException', file_get_contents($filePath), ); - self::assertFalse($report->hasViolations()); + self::assertFalse($report->hasFinalViolations()); + self::assertSame(1, $report->fixingPasses); } finally { @unlink($filePath); } @@ -97,7 +102,7 @@ public function itReportsRemainingViolationsAtTheirFinalLines(): void $filePath = $this->temporaryFile($source); try { - $lineBreakSniff = new class (RunMode::Fix) extends AbstractSniff implements Fixable { + $lineBreakSniff = new class extends AbstractSniff implements Fixable { private const string ELEMENT = ''; public static function getCode(): string @@ -129,7 +134,7 @@ public function process(\DOMDocument $document, File $file): array )]; } }; - $badElementSniff = new class (RunMode::Fix) extends AbstractSniff { + $badElementSniff = new class extends AbstractSniff { public static function getCode(): string { return 'Test.BadElement'; @@ -154,16 +159,16 @@ public function process(\DOMDocument $document, File $file): array )]; } }; - $processor = new XmlFileProcessor([ + $processor = new XmlFileProcessor(new XmlSniffRunner(RunMode::Fix, [ $lineBreakSniff, $badElementSniff, - ]); + ])); $report = $this->processFile($processor, $filePath); self::assertSame("\n", file_get_contents($filePath)); - self::assertSame(1, $report->getViolationCount()); - self::assertSame(2, $report->violations[0]->rangeOne()->line); + self::assertSame(1, $report->getFinalViolationCount()); + self::assertSame(2, $report->finalViolations[0]->rangeOne()->line); } finally { @unlink($filePath); } @@ -176,7 +181,7 @@ public function itKeepsChangedLineScopeAlignedAfterFixes(): void $filePath = $this->temporaryFile($source); try { - $lineBreakSniff = new class (RunMode::Fix) extends AbstractSniff implements Fixable { + $lineBreakSniff = new class extends AbstractSniff implements Fixable { public static function getCode(): string { return 'Test.ScopedLineBreak'; @@ -202,7 +207,7 @@ public function process(\DOMDocument $document, File $file): array )]; } }; - $badElementSniff = new class (RunMode::Fix) extends AbstractSniff { + $badElementSniff = new class extends AbstractSniff { public static function getCode(): string { return 'Test.ScopedBadElement'; @@ -224,7 +229,9 @@ public function process(\DOMDocument $document, File $file): array )]; } }; - $processor = new XmlFileProcessor([$lineBreakSniff, $badElementSniff]); + $processor = new XmlFileProcessor( + new XmlSniffRunner(RunMode::Fix, [$lineBreakSniff, $badElementSniff]) + ); $report = $this->processFile( $processor, @@ -233,8 +240,8 @@ public function process(\DOMDocument $document, File $file): array ); self::assertSame("\n\n\n", file_get_contents($filePath)); - self::assertSame(1, $report->getViolationCount()); - self::assertSame(3, $report->violations[0]->rangeOne()->line); + self::assertSame(1, $report->getFinalViolationCount()); + self::assertSame(3, $report->finalViolations[0]->rangeOne()->line); } finally { @unlink($filePath); } @@ -247,7 +254,7 @@ public function itDoesNotPersistFixesThatCycle(): void $filePath = $this->temporaryFile($source); try { - $toggleElementSniff = new class (RunMode::Fix) extends AbstractSniff implements Fixable { + $toggleElementSniff = new class extends AbstractSniff implements Fixable { public static function getCode(): string { return 'Test.ToggleElement'; @@ -273,9 +280,9 @@ public function process(\DOMDocument $document, File $file): array )]; } }; - $processor = new XmlFileProcessor([ + $processor = new XmlFileProcessor(new XmlSniffRunner(RunMode::Fix, [ $toggleElementSniff, - ]); + ])); try { $this->processFile($processor, $filePath); @@ -297,7 +304,7 @@ public function itDoesNotPersistFixesThatProduceInvalidXml(): void $filePath = $this->temporaryFile($source); try { - $invalidXmlSniff = new class (RunMode::Fix) extends AbstractSniff implements Fixable { + $invalidXmlSniff = new class extends AbstractSniff implements Fixable { public static function getCode(): string { return 'Test.InvalidXml'; @@ -322,7 +329,7 @@ public function process(\DOMDocument $document, File $file): array )]; } }; - $processor = new XmlFileProcessor([$invalidXmlSniff]); + $processor = new XmlFileProcessor(new XmlSniffRunner(RunMode::Fix, [$invalidXmlSniff])); try { $this->processFile($processor, $filePath); @@ -342,12 +349,17 @@ private function processFile(XmlFileProcessor $processor, string $path, ?FileCha $content = file_get_contents($path); self::assertIsString($content); - $result = $processor->process(new File($path, $content), $fileChange); - if ($result->isModified()) { - file_put_contents($path, $result->fixedContent()); + $fixedFile = $processor->process( + $file = new File($path, $content), + $fileReport = new FileReport($path), + RunScope::fromFileAndFileChange($file, $fileChange) + ); + + if ($fixedFile !== null) { + file_put_contents($path, $fixedFile->content); } - return $result->fileReport; + return $fileReport; } private function temporaryFile(string $content): string diff --git a/tests/Unit/Runner/RunCoordinatorFileFailureTest.php b/tests/Unit/Runner/RunCoordinatorFileFailureTest.php index 477509b..c0b1385 100644 --- a/tests/Unit/Runner/RunCoordinatorFileFailureTest.php +++ b/tests/Unit/Runner/RunCoordinatorFileFailureTest.php @@ -5,17 +5,92 @@ namespace DocbookCS\Tests\Unit\Runner; use DocbookCS\Config\ConfigData; +use DocbookCS\Config\SniffEntry; +use DocbookCS\Diff\DiffBaseResolver; use DocbookCS\Diff\DiffChangeset; use DocbookCS\Diff\FileChange; +use DocbookCS\Diff\GitDiffProvider; +use DocbookCS\Diff\UpstreamResolver; +use DocbookCS\Fix\Fix; +use DocbookCS\Fix\FixApplier; +use DocbookCS\Fix\FixPlan; +use DocbookCS\Fix\FixResult; +use DocbookCS\Fix\Fixer\SimparaFixer; +use DocbookCS\Fix\FixerException; +use DocbookCS\Git\GitClient; +use DocbookCS\Path\DiffPathLoader; +use DocbookCS\Path\EntityResolver; +use DocbookCS\Path\PathLoader; +use DocbookCS\Path\PathMatcher; +use DocbookCS\Progress\NullProgress; use DocbookCS\Progress\ProgressInterface; +use DocbookCS\Report\FileReport; +use DocbookCS\Report\Report; +use DocbookCS\Runner\EntityExpansionMarker; +use DocbookCS\Runner\EntityPreprocessor; use DocbookCS\Runner\RunCoordinator; +use DocbookCS\Runner\RunMode; use DocbookCS\Runner\RunPlan; use DocbookCS\Runner\RunPlanner; -use PHPUnit\Framework\Attributes\CoversNothing; +use DocbookCS\Runner\RunScope; +use DocbookCS\Runner\RunScopeResolver; +use DocbookCS\Runner\ViolationScopeFilter; +use DocbookCS\Runner\XmlFileProcessor; +use DocbookCS\Runner\XmlFixRunner; +use DocbookCS\Runner\XmlSniffRunner; +use DocbookCS\Sniff\AbstractSniff; +use DocbookCS\Sniff\SimparaSniff; +use DocbookCS\Source\File; +use DocbookCS\Violation\SourceRange; +use DocbookCS\Violation\Violation; +use DocbookCS\Xml\XmlParser; +use PHPUnit\Framework\Attributes\CoversClass; use PHPUnit\Framework\Attributes\Test; +use PHPUnit\Framework\Attributes\UsesClass; use PHPUnit\Framework\TestCase; -#[CoversNothing] +#[ + CoversClass(FixerException::class), + CoversClass(RunCoordinator::class), + // + UsesClass(AbstractSniff::class), + UsesClass(ConfigData::class), + UsesClass(DiffBaseResolver::class), + UsesClass(DiffChangeset::class), + UsesClass(DiffPathLoader::class), + UsesClass(EntityExpansionMarker::class), + UsesClass(EntityPreprocessor::class), + UsesClass(EntityResolver::class), + UsesClass(File::class), + UsesClass(FileChange::class), + UsesClass(FileReport::class), + UsesClass(Fix::class), + UsesClass(FixApplier::class), + UsesClass(FixPlan::class), + UsesClass(FixResult::class), + UsesClass(GitClient::class), + UsesClass(GitDiffProvider::class), + UsesClass(NullProgress::class), + UsesClass(PathLoader::class), + UsesClass(PathMatcher::class), + UsesClass(Report::class), + UsesClass(RunMode::class), + UsesClass(RunPlan::class), + UsesClass(RunPlanner::class), + UsesClass(RunScope::class), + UsesClass(RunScopeResolver::class), + UsesClass(SimparaFixer::class), + UsesClass(SimparaSniff::class), + UsesClass(SniffEntry::class), + UsesClass(SourceRange::class), + UsesClass(UpstreamResolver::class), + UsesClass(Violation::class), + UsesClass(ViolationScopeFilter::class), + UsesClass(XmlFileProcessor::class), + UsesClass(XmlFixRunner::class), + UsesClass(XmlParser::class), + UsesClass(XmlSniffRunner::class), +] final class RunCoordinatorFileFailureTest extends TestCase { #[Test] @@ -55,9 +130,11 @@ public function finish(): void basePath: dirname($xmlFilePath), ); - $report = new RunCoordinator($progress)->run($this->planPaths($config)); + $report = new RunCoordinator($progress)->runWithMetrics( + new RunPlanner($config)->planPaths($config->getIncludePaths()), + ); - self::assertTrue($report->hasViolations()); + self::assertTrue($report->hasFinalViolations()); self::assertSame('DocbookCS.Internal', $report->getAllViolations()[0]->sniffCode); self::assertStringContainsString('Could not read file', $report->getAllViolations()[0]->message); } @@ -89,19 +166,39 @@ static function () use ($xmlFilePath): void { ); $diff = new DiffChangeset([new FileChange($xmlFilePath, [42])]); - $report = new RunCoordinator($progress)->run($this->planDiff($config, $diff)); + $report = new RunCoordinator($progress)->runWithMetrics( + new RunPlanner($config)->planDiff($diff), + ); - self::assertTrue($report->hasViolations()); + self::assertTrue($report->hasFinalViolations()); self::assertSame('DocbookCS.Internal', $report->getAllViolations()[0]->sniffCode); } - private function planPaths(ConfigData $config): RunPlan + #[Test] + public function itFailsClearlyWhenFixedContentCannotBePersisted(): void { - return new RunPlanner($config)->planPaths($config->getIncludePaths()); - } + if (PHP_OS_FAMILY === 'Windows') { + self::markTestSkipped('Unix file permissions are required.'); + } - private function planDiff(ConfigData $config, DiffChangeset $diff): RunPlan - { - return new RunPlanner($config)->planDiff($diff); + $filePath = tempnam(sys_get_temp_dir(), 'docbook-cs-'); + self::assertIsString($filePath); + file_put_contents($filePath, 'Text'); + chmod($filePath, 0444); + + try { + $this->expectException(FixerException::class); + $this->expectExceptionMessageIs('Could not write fixed file: ' . $filePath); + + new RunCoordinator()->runWithMetrics(new RunPlan( + mode: RunMode::Fix, + sniffs: [new SniffEntry(SimparaSniff::class)], + targets: [$filePath => null], + entities: [], + )); + } finally { + chmod($filePath, 0644); + @unlink($filePath); + } } } diff --git a/tests/Unit/Runner/RunReportingTest.php b/tests/Unit/Runner/RunReportingTest.php new file mode 100644 index 0000000..5bb7e7c --- /dev/null +++ b/tests/Unit/Runner/RunReportingTest.php @@ -0,0 +1,113 @@ +Text'); + + try { + $plan = new RunPlan( + mode: RunMode::Fix, + sniffs: [new SniffEntry(SimparaSniff::class)], + targets: [$filePath => null], + entities: [], + ); + + $report = new RunCoordinator(collectPerformance: true)->runWithMetrics($plan); + + self::assertSame('Text', file_get_contents($filePath)); + self::assertSame(1, $report->getChangedFilesCount()); + self::assertSame(1, $report->getFoundViolationsCount()); + self::assertSame(1, $report->getAppliedFixesCount()); + self::assertSame(0, $report->getSkippedFixesCount()); + self::assertSame(1, $report->getFixingPassesCount()); + self::assertFalse($report->hasFinalViolations()); + self::assertArrayHasKey(SimparaSniff::getCode(), $report->getSniffingTimes()); + self::assertArrayHasKey(SimparaSniff::getCode(), $report->getFixingTimes()); + self::assertGreaterThanOrEqual( + array_sum($report->getSniffingTimes()), + $report->getTotalSniffingTime(), + ); + self::assertGreaterThan(0.0, $report->getTotalFixingTime()); + } finally { + @unlink($filePath); + } + } + + #[Test] + public function itReportsInitialAndFinalViolationsAfterFixingShiftsTheSource(): void + { + $filePath = tempnam(sys_get_temp_dir(), 'docbook-cs-reporting-'); + self::assertIsString($filePath); + file_put_contents($filePath, 'Text'); + + $badElementSniff = new class () extends AbstractSniff { + public static function getCode(): string + { + return 'Test.BadElement'; + } + + public function process(\DOMDocument $document, File $file): array + { + $offset = strpos($file->content, ''); + if ($offset === false) { + return []; + } + + return [$this->createViolation( + $file->path, + 'Bad element.', + [new SourceRange(1, $offset, $offset + strlen(''), '')], + )]; + } + }; + + try { + $report = new RunCoordinator()->runWithMetrics(new RunPlan( + mode: RunMode::Fix, + sniffs: [ + new SniffEntry(SimparaSniff::class), + new SniffEntry($badElementSniff::class), + ], + targets: [$filePath => null], + entities: [], + )); + + $fileReport = $report->fileReports[$filePath]; + + self::assertSame(2, $fileReport->getFoundViolationCount()); + self::assertSame(SimparaSniff::getCode(), $fileReport->foundViolations[0]->sniffCode); + self::assertSame(1, $fileReport->getFinalViolationCount()); + self::assertSame('Test.BadElement', $fileReport->finalViolations[0]->sniffCode); + self::assertGreaterThan( + $fileReport->foundViolations[1]->rangeOne()->beginOffset, + $fileReport->finalViolations[0]->rangeOne()->beginOffset, + ); + self::assertSame(1, $report->getAppliedFixesCount()); + self::assertSame(1, $report->getSkippedFixesCount()); + } finally { + @unlink($filePath); + } + } +} diff --git a/tests/Unit/Runner/RunScopeResolverTest.php b/tests/Unit/Runner/RunScopeResolverTest.php index 75d97c2..c914f00 100644 --- a/tests/Unit/Runner/RunScopeResolverTest.php +++ b/tests/Unit/Runner/RunScopeResolverTest.php @@ -161,6 +161,31 @@ public function wideScopeHandlesCyclicEntityReferences(): void ); } + #[Test] + public function wideScopeSkipsExpansionForUnreadableFiles(): void + { + if (DIRECTORY_SEPARATOR === '\\') { + self::markTestSkipped('Windows does not support Unix file permissions.'); + } + + chmod($this->sourceFile, 0000); + clearstatcache(true, $this->sourceFile); + + if (is_readable($this->sourceFile)) { + chmod($this->sourceFile, 0600); + self::markTestSkipped('The current user can read files without permissions.'); + } + + try { + self::assertSame( + [$this->sourceFile => null], + $this->resolver(wide: true)->resolvePaths([$this->sourceFile]), + ); + } finally { + chmod($this->sourceFile, 0600); + } + } + #[Test] public function wideScopeNormalisesParentDirectorySegmentsInEntityPaths(): void { diff --git a/tests/Unit/Runner/RunScopeTest.php b/tests/Unit/Runner/RunScopeTest.php index ad5e580..cc864d9 100644 --- a/tests/Unit/Runner/RunScopeTest.php +++ b/tests/Unit/Runner/RunScopeTest.php @@ -26,6 +26,8 @@ use DocbookCS\Report\Report; use DocbookCS\Runner\EntityExpansionMarker; use DocbookCS\Runner\EntityPreprocessor; +use DocbookCS\Runner\XmlFileProcessor; +use DocbookCS\Runner\XmlFixRunner; use DocbookCS\Runner\RunCoordinator; use DocbookCS\Runner\RunMode; use DocbookCS\Runner\RunPlan; @@ -33,14 +35,14 @@ use DocbookCS\Runner\RunScope; use DocbookCS\Runner\RunScopeResolver; use DocbookCS\Runner\ViolationScopeFilter; -use DocbookCS\Runner\XmlFileProcessor; -use DocbookCS\Runner\XmlProcessingResult; +use DocbookCS\Runner\XmlSniffRunner; use DocbookCS\Sniff\AbstractSniff; use DocbookCS\Sniff\SimparaSniff; use DocbookCS\Source\File; use DocbookCS\Source\Line; use DocbookCS\Violation\SourceRange; use DocbookCS\Violation\Violation; +use DocbookCS\Xml\XmlParser; use PHPUnit\Framework\Attributes\CoversClass; use PHPUnit\Framework\Attributes\Test; use PHPUnit\Framework\Attributes\UsesClass; @@ -84,7 +86,9 @@ UsesClass(Violation::class), UsesClass(ViolationScopeFilter::class), UsesClass(XmlFileProcessor::class), - UsesClass(XmlProcessingResult::class), + UsesClass(XmlFixRunner::class), + UsesClass(XmlParser::class), + UsesClass(XmlSniffRunner::class), ] final class RunScopeTest extends TestCase { @@ -120,14 +124,14 @@ public function itExpandsReferencedTargetsOnlyWhenWideScopeIsRequested(): void { $config = $this->config(); - self::assertSame(1, $this->executePaths($config, [$this->sourceFile])->getFilesScanned()); + self::assertSame(1, $this->executePaths($config, [$this->sourceFile])->getScannedFilesCount()); self::assertSame( 2, $this->executePaths( $config, [$this->sourceFile], wide: true, - )->getFilesScanned(), + )->getScannedFilesCount(), ); } @@ -148,7 +152,7 @@ public function itFixesExpandedXmlInItsTargetFileOnly(): void self::assertSame('⌖', file_get_contents($this->sourceFile)); self::assertSame('Text', file_get_contents($this->targetFile)); - self::assertFalse($report->hasViolations()); + self::assertFalse($report->hasFinalViolations()); } #[Test] @@ -163,14 +167,13 @@ public function aDiffProvidesItsOwnFilesWithoutConfiguredIncludePaths(): void basePath: $this->directory, ); - $report = $this->executeDiff( - $config, - new DiffChangeset([ + $report = new RunCoordinator()->runWithMetrics( + new RunPlanner($config)->planDiff(new DiffChangeset([ new FileChange($this->sourceFile, [1]), - ]), + ])), ); - self::assertSame(1, $report->getFilesScanned()); + self::assertSame(1, $report->getScannedFilesCount()); } #[Test] @@ -185,14 +188,13 @@ public function aDiffPathUsingAProjectDirectoryKeepsItsSourceRanges(): void basePath: $this->directory, ); - $report = $this->executeDiff( - $config, - new DiffChangeset([ + $report = new RunCoordinator()->runWithMetrics( + new RunPlanner($config)->planDiff(new DiffChangeset([ new FileChange('docs/source.xml', [1]), - ]), + ])), ); - self::assertSame(1, $report->getFilesScanned()); + self::assertSame(1, $report->getScannedFilesCount()); } /** @param list $sniffs */ @@ -215,19 +217,8 @@ private function executePaths( RunMode $mode = RunMode::Sniff, bool $wide = false, ): Report { - return new RunCoordinator()->run( + return new RunCoordinator()->runWithMetrics( new RunPlanner($config, $mode, $wide)->planPaths($paths), ); } - - private function executeDiff( - ConfigData $config, - DiffChangeset $diff, - RunMode $mode = RunMode::Sniff, - bool $wide = false, - ): Report { - return new RunCoordinator()->run( - new RunPlanner($config, $mode, $wide)->planDiff($diff), - ); - } } diff --git a/tests/Unit/Runner/SniffRunnerTest.php b/tests/Unit/Runner/SniffRunnerTest.php index 44657f3..3bafe65 100644 --- a/tests/Unit/Runner/SniffRunnerTest.php +++ b/tests/Unit/Runner/SniffRunnerTest.php @@ -22,6 +22,7 @@ use DocbookCS\Report\Report; use DocbookCS\Runner\EntityExpansionMarker; use DocbookCS\Runner\EntityPreprocessor; +use DocbookCS\Runner\XmlFileProcessor; use DocbookCS\Runner\RunCoordinator; use DocbookCS\Runner\RunMode; use DocbookCS\Runner\RunPlan; @@ -29,14 +30,14 @@ use DocbookCS\Runner\RunScope; use DocbookCS\Runner\RunScopeResolver; use DocbookCS\Runner\ViolationScopeFilter; -use DocbookCS\Runner\XmlFileProcessor; -use DocbookCS\Runner\XmlProcessingResult; +use DocbookCS\Runner\XmlSniffRunner; use DocbookCS\Sniff\SniffInterface; use DocbookCS\Source\File; use DocbookCS\Source\Line; use DocbookCS\Violation\Severity; use DocbookCS\Violation\SourceRange; use DocbookCS\Violation\Violation; +use DocbookCS\Xml\XmlParser; use PHPUnit\Framework\Attributes\CoversClass; use PHPUnit\Framework\Attributes\Test; use PHPUnit\Framework\Attributes\UsesClass; @@ -57,7 +58,7 @@ CoversClass(RunPlanner::class), CoversClass(SniffEntry::class), CoversClass(Violation::class), - CoversClass(XmlFileProcessor::class), + CoversClass(XmlSniffRunner::class), // UsesClass(DiffBaseResolver::class), UsesClass(DiffChangeset::class), @@ -73,7 +74,8 @@ UsesClass(SourceRange::class), UsesClass(UpstreamResolver::class), UsesClass(ViolationScopeFilter::class), - UsesClass(XmlProcessingResult::class), + UsesClass(XmlFileProcessor::class), + UsesClass(XmlParser::class), ] final class SniffRunnerTest extends TestCase { @@ -92,34 +94,33 @@ private function createConfig(array $sniffs = []): ConfigData ); } - #[Test] // TODO: should be integration + #[Test] public function itProcessesFilesWithoutViolations(): void { $config = $this->createConfig(); $runner = new RunCoordinator(); - $report = $runner->run($this->planPaths($config)); + $report = $runner->runWithMetrics(new RunPlanner($config)->planPaths($config->getIncludePaths())); - self::assertSame(2, $report->getFilesScanned()); - self::assertFalse($report->hasViolations()); - self::assertCount(0, $report->getFileReports()); + self::assertSame(2, $report->getScannedFilesCount()); + self::assertFalse($report->hasFinalViolations()); + self::assertCount(2, $report->fileReports); } - #[Test] // TODO: should be integration + #[Test] public function itUsesOverridePathsWhenProvided(): void { $config = $this->createConfig(); $runner = new RunCoordinator(); - $report = $runner->run($this->planPaths( - $config, + $report = $runner->runWithMetrics(new RunPlanner($config)->planPaths( [self::FIXTURE_DIR . '/../override'], )); - self::assertSame(1, $report->getFilesScanned()); + self::assertSame(1, $report->getScannedFilesCount()); } - #[Test] // TODO: should be integration + #[Test] public function itCallsProgressMethods(): void { $progress = $this->createMock(ProgressInterface::class); @@ -137,17 +138,13 @@ public function itCallsProgressMethods(): void $config = $this->createConfig(); $runner = new RunCoordinator($progress); - $runner->run($this->planPaths($config)); + $runner->runWithMetrics(new RunPlanner($config)->planPaths($config->getIncludePaths())); } - #[Test] // TODO: should be integration + #[Test] public function itAddsFileReportsForFilesWithViolations(): void { - $sniff = new class (RunMode::Sniff) implements SniffInterface { - public function __construct(public RunMode $mode) - { - } - + $sniff = new /** @implements SniffInterface */ class implements SniffInterface { public static function getCode(): string { return 'Test.ViolatingSniff'; @@ -174,21 +171,17 @@ public function setProperty(string $name, string $value): void $config = $this->createConfig(sniffs: [new SniffEntry($sniff::class)]); $runner = new RunCoordinator(); - $report = $runner->run($this->planPaths($config)); + $report = $runner->runWithMetrics(new RunPlanner($config)->planPaths($config->getIncludePaths())); - self::assertSame(2, $report->getFilesScanned()); - self::assertCount(2, $report->getFileReports()); - self::assertTrue($report->hasViolations()); + self::assertSame(2, $report->getScannedFilesCount()); + self::assertCount(2, $report->fileReports); + self::assertTrue($report->hasFinalViolations()); } - #[Test] // TODO: should be integration + #[Test] public function itStoresAbsolutePathsInFileReports(): void { - $sniff = new class (RunMode::Sniff) implements SniffInterface { - public function __construct(public RunMode $mode) - { - } - + $sniff = new /** @implements SniffInterface */ class implements SniffInterface { public static function getCode(): string { return 'Test.ViolatingSniff'; @@ -215,9 +208,9 @@ public function setProperty(string $name, string $value): void $config = $this->createConfig(sniffs: [new SniffEntry($sniff::class)]); $runner = new RunCoordinator(); - $report = $runner->run($this->planPaths($config)); + $report = $runner->runWithMetrics(new RunPlanner($config)->planPaths($config->getIncludePaths())); - foreach ($report->getFileReports() as $fileReport) { + foreach ($report->fileReports as $fileReport) { self::assertTrue( str_starts_with($fileReport->filePath, '/'), 'Expected absolute path, got: ' . $fileReport->filePath, @@ -225,17 +218,11 @@ public function setProperty(string $name, string $value): void } } - #[Test] // TODO: should be integration + #[Test] public function itPassesPropertiesToSniffs(): void { - $sniffClass = new class (RunMode::Sniff) implements SniffInterface { + $sniffClass = new class implements SniffInterface { public static string $captured = ''; - public static RunMode $capturedMode = RunMode::Sniff; - - public function __construct(public RunMode $mode) - { - self::$capturedMode = $mode; - } public function setProperty(string $name, string $value): void { @@ -256,13 +243,12 @@ public function process(\DOMDocument $document, File $file): array $config = $this->createConfig(sniffs: [new SniffEntry($sniffClass::class, ['someProp' => 'someValue'])]); $runner = new RunCoordinator(); - $runner->run($this->planPaths($config, mode: RunMode::Fix)); + $runner->runWithMetrics(new RunPlanner($config)->planPaths($config->getIncludePaths())); self::assertSame('someValue', $sniffClass::$captured); - self::assertSame(RunMode::Fix, $sniffClass::$capturedMode); } - #[Test] // TODO: should be integration + #[Test] public function itThrowsWhenSniffClassDoesNotExist(): void { $config = $this->createConfig(sniffs: [new SniffEntry('NonExistent\\FakeSniff')]); @@ -272,10 +258,10 @@ public function itThrowsWhenSniffClassDoesNotExist(): void $this->expectException(\RuntimeException::class); $this->expectExceptionMessageIsOrContains('does not exist'); - $runner->run($this->planPaths($config)); + $runner->runWithMetrics(new RunPlanner($config)->planPaths($config->getIncludePaths())); } - #[Test] // TODO: should be integration + #[Test] public function itThrowsWhenClassDoesNotImplementSniffInterface(): void { $config = $this->createConfig(sniffs: [new SniffEntry(\stdClass::class)]); @@ -285,34 +271,34 @@ public function itThrowsWhenClassDoesNotImplementSniffInterface(): void $this->expectException(\RuntimeException::class); $this->expectExceptionMessageIsOrContains('does not implement'); - $runner->run($this->planPaths($config)); + $runner->runWithMetrics(new RunPlanner($config)->planPaths($config->getIncludePaths())); } - #[Test] // TODO: should be integration + #[Test] public function itFiltersFilesToOnlyThoseInTheDiff(): void { $config = $this->createConfig(); $runner = new RunCoordinator(); $diff = new DiffChangeset([new FileChange(self::FIXTURE_DIR . '/file_a.xml', [1])]); - $report = $runner->run($this->planDiff($config, $diff)); + $report = $runner->runWithMetrics(new RunPlanner($config)->planDiff($diff)); - self::assertSame(1, $report->getFilesScanned()); + self::assertSame(1, $report->getScannedFilesCount()); } - #[Test] // TODO: should be integration + #[Test] public function itScansNoFilesWhenDiffContainsNoMatchingPaths(): void { $config = $this->createConfig(); $runner = new RunCoordinator(); $diff = new DiffChangeset([new FileChange('completely/different/file.xml', [1, 2, 3])]); - $report = $runner->run($this->planDiff($config, $diff)); + $report = $runner->runWithMetrics(new RunPlanner($config)->planDiff($diff)); - self::assertSame(0, $report->getFilesScanned()); + self::assertSame(0, $report->getScannedFilesCount()); } - #[Test] // TODO: should be integration + #[Test] public function itMatchesWhenDiffPathEqualsDiscoveredPath(): void { $config = $this->createConfig(); @@ -321,23 +307,23 @@ public function itMatchesWhenDiffPathEqualsDiscoveredPath(): void $discoveredPath = self::FIXTURE_DIR . '/file_a.xml'; $diff = new DiffChangeset([new FileChange($discoveredPath, [1])]); - $report = $runner->run($this->planDiff($config, $diff)); + $report = $runner->runWithMetrics(new RunPlanner($config)->planDiff($diff)); - self::assertSame(1, $report->getFilesScanned()); + self::assertSame(1, $report->getScannedFilesCount()); } - #[Test] // TODO: should be integration + #[Test] public function itScansAllFilesWhenNoDiffIsGiven(): void { $config = $this->createConfig(); $runner = new RunCoordinator(); - $report = $runner->run($this->planPaths($config)); + $report = $runner->runWithMetrics(new RunPlanner($config)->planPaths($config->getIncludePaths())); - self::assertSame(2, $report->getFilesScanned()); + self::assertSame(2, $report->getScannedFilesCount()); } - #[Test] // TODO: should be integration + #[Test] public function itScansLexicallyEquivalentWideTargetsOnlyOnce(): void { $directory = sys_get_temp_dir() . '/docbook-cs-scan-' . bin2hex(random_bytes(6)); @@ -371,9 +357,9 @@ public function itScansLexicallyEquivalentWideTargetsOnlyOnce(): void ], ); - $report = new RunCoordinator()->run($plan); + $report = new RunCoordinator()->runWithMetrics($plan); - self::assertSame(2, $report->getFilesScanned()); + self::assertSame(2, $report->getScannedFilesCount()); } finally { @unlink($sourceFile); @unlink($targetFile); @@ -382,14 +368,10 @@ public function itScansLexicallyEquivalentWideTargetsOnlyOnce(): void } } - #[Test] // TODO: should be integration + #[Test] public function itReportsNoViolationsForFilesInDiffWithoutAddedLines(): void { - $sniff = new class (RunMode::Sniff) implements SniffInterface { - public function __construct(public RunMode $mode) - { - } - + $sniff = new /** @implements SniffInterface */ class implements SniffInterface { public static function getCode(): string { return 'Test.ViolatingSniff'; @@ -417,20 +399,9 @@ public function setProperty(string $name, string $value): void $runner = new RunCoordinator(); $diff = new DiffChangeset([new FileChange(self::FIXTURE_DIR . '/file_a.xml', [])]); - $report = $runner->run($this->planDiff($config, $diff)); - - self::assertSame(1, $report->getFilesScanned()); - self::assertFalse($report->hasViolations()); - } + $report = $runner->runWithMetrics(new RunPlanner($config)->planDiff($diff)); - /** @param list|null $paths */ - private function planPaths(ConfigData $config, ?array $paths = null, RunMode $mode = RunMode::Sniff): RunPlan - { - return new RunPlanner($config, $mode)->planPaths($paths ?? $config->getIncludePaths()); - } - - private function planDiff(ConfigData $config, DiffChangeset $diff, RunMode $mode = RunMode::Sniff): RunPlan - { - return new RunPlanner($config, $mode)->planDiff($diff); + self::assertSame(1, $report->getScannedFilesCount()); + self::assertFalse($report->hasFinalViolations()); } } diff --git a/tests/Unit/Runner/SourceRangeScopeTest.php b/tests/Unit/Runner/SourceRangeScopeTest.php index 89ab020..3a226c6 100644 --- a/tests/Unit/Runner/SourceRangeScopeTest.php +++ b/tests/Unit/Runner/SourceRangeScopeTest.php @@ -7,32 +7,35 @@ use DocbookCS\Diff\FileChange; use DocbookCS\Fix\Fix; use DocbookCS\Fix\FixApplier; +use DocbookCS\Fix\Fixer\SimparaFixer; use DocbookCS\Fix\FixPlan; use DocbookCS\Fix\FixResult; -use DocbookCS\Fix\Fixer\SimparaFixer; use DocbookCS\Report\FileReport; use DocbookCS\Report\Report; use DocbookCS\Runner\EntityExpansionMarker; use DocbookCS\Runner\EntityPreprocessor; +use DocbookCS\Runner\XmlFileProcessor; +use DocbookCS\Runner\XmlFixRunner; use DocbookCS\Runner\RunMode; use DocbookCS\Runner\RunScope; use DocbookCS\Runner\ViolationScopeFilter; -use DocbookCS\Runner\XmlFileProcessor; -use DocbookCS\Runner\XmlProcessingResult; +use DocbookCS\Runner\XmlSniffRunner; use DocbookCS\Sniff\SimparaSniff; use DocbookCS\Source\File; use DocbookCS\Source\Line; use DocbookCS\Violation\SourceRange; use DocbookCS\Violation\Violation; +use DocbookCS\Xml\XmlParser; use PHPUnit\Framework\Attributes\CoversClass; use PHPUnit\Framework\Attributes\Test; use PHPUnit\Framework\Attributes\UsesClass; use PHPUnit\Framework\TestCase; #[ - CoversClass(SimparaSniff::class), CoversClass(RunScope::class), + CoversClass(SimparaSniff::class), CoversClass(XmlFileProcessor::class), + CoversClass(XmlSniffRunner::class), // UsesClass(EntityExpansionMarker::class), UsesClass(EntityPreprocessor::class), @@ -50,7 +53,8 @@ UsesClass(SourceRange::class), UsesClass(Violation::class), UsesClass(ViolationScopeFilter::class), - UsesClass(XmlProcessingResult::class), + UsesClass(XmlFixRunner::class), + UsesClass(XmlParser::class), ] final class SourceRangeScopeTest extends TestCase { @@ -76,19 +80,20 @@ public function itAppliesEveryRangeOfAViolationIntersectingAChangedLine(): void file_put_contents($filePath, $source); try { - $processor = new XmlFileProcessor([ - new SimparaSniff(RunMode::Fix), - ]); + $processor = new XmlFileProcessor(new XmlSniffRunner(RunMode::Fix, [ + new SimparaSniff(), + ])); - $result = $processor->process( - new File($filePath, $source), - new FileChange($filePath, [3]), + $fixedFile = $processor->process( + $file = new File($filePath, $source), + $fileReport = new FileReport($filePath), + RunScope::fromFileAndFileChange($file, new FileChange($filePath, [3])), ); - file_put_contents($filePath, $result->fixedContent()); - $report = $result->fileReport; + self::assertNotNull($fixedFile); + file_put_contents($filePath, $fixedFile->content); self::assertSame($expected, file_get_contents($filePath)); - self::assertFalse($report->hasViolations()); + self::assertFalse($fileReport->hasFinalViolations()); } finally { @unlink($filePath); } @@ -111,16 +116,18 @@ public function itFixesAViolationCausedByADeletedLine(): void XML; - $processor = new XmlFileProcessor([ - new SimparaSniff(RunMode::Fix), - ]); + $processor = new XmlFileProcessor(new XmlSniffRunner(RunMode::Fix, [ + new SimparaSniff(), + ])); - $result = $processor->process( - new File('file.xml', $source), - new FileChange('file.xml', [], deletionAnchors: [3]), + $fixedFile = $processor->process( + $file = new File('file.xml', $source), + $fileReport = new FileReport('file.xml'), + RunScope::fromFileAndFileChange($file, new FileChange('file.xml', [], deletionAnchors: [3])), ); - self::assertSame($expected, $result->fixedContent()); - self::assertFalse($result->fileReport->hasViolations()); + self::assertNotNull($fixedFile); + self::assertSame($expected, $fixedFile->content); + self::assertFalse($fileReport->hasFinalViolations()); } } diff --git a/tests/Unit/Runner/SourceScopeTest.php b/tests/Unit/Runner/SourceScopeTest.php index 0c73508..3d74c0a 100644 --- a/tests/Unit/Runner/SourceScopeTest.php +++ b/tests/Unit/Runner/SourceScopeTest.php @@ -36,6 +36,7 @@ public function itIncludesOnlyViolationsIntersectingChangedLines(): void self::assertFalse($scope->isWholeFile()); self::assertSame([2], $scope->lineNumbers($file)); + self::assertTrue($scope->includes($this->violation(5, 5, 2))); self::assertTrue($scope->includes($this->violation(4, 7, 2))); self::assertFalse($scope->includes($this->violation(8, 13, 3))); } @@ -48,6 +49,10 @@ public function wholeFileScopeIncludesEveryLine(): void self::assertTrue($scope->isWholeFile()); self::assertSame([1, 2, 3], $scope->lineNumbers($file)); + self::assertTrue($scope->includes($this->violation(8, 13, 3))); + self::assertSame($scope, $scope->after([ + new Fix('file.xml', 0, 0, "zero\n", 'Test'), + ])); } #[Test] @@ -57,6 +62,31 @@ public function anEmptyChangedLineSetSelectsNoLines(): void $scope = RunScope::fromFileAndFileChange($file, new FileChange('file.xml', [])); self::assertSame([], $scope->lineNumbers($file)); + self::assertSame($scope, $scope->after([ + new Fix('file.xml', 0, 0, "zero\n", 'Test'), + ])); + } + + #[Test] + public function itIgnoresDeletionAnchorsOutsideTheSource(): void + { + $file = new File('file.xml', "one\ntwo\nthree"); + $scope = RunScope::fromFileAndFileChange( + $file, + new FileChange('file.xml', [], deletionAnchors: [10]), + ); + + self::assertSame([], $scope->lineNumbers($file)); + } + + #[Test] + public function itCombinesAdjacentChangedLines(): void + { + $file = new File('file.xml', "one\ntwo\nthree"); + $scope = RunScope::fromFileAndFileChange($file, new FileChange('file.xml', [2, 3])); + + self::assertSame([2, 3], $scope->lineNumbers($file)); + self::assertTrue($scope->includes($this->violation(7, 9, 2))); } #[Test] @@ -87,6 +117,19 @@ public function itIncludesContentInsertedIntoAnEmptySelectedLine(): void self::assertTrue($scope->includes($this->violation(5, 10, 2))); } + #[Test] + public function itKeepsScopeAlignedWhenAReplacementSpansItsBeginning(): void + { + $file = new File('file.xml', "one\ntwo\nthree"); + $scope = RunScope::fromFileAndFileChange($file, new FileChange('file.xml', [2]))->after([ + new Fix('file.xml', 2, 6, 'X', 'Test'), + ]); + + self::assertSame([1], $scope->lineNumbers($file->withContent("onXo\nthree"))); + self::assertTrue($scope->includes($this->violation(2, 5, 1))); + self::assertFalse($scope->includes($this->violation(5, 10, 2))); + } + #[Test] public function itSortsFixesBeforeMappingScopeOffsets(): void { @@ -123,7 +166,10 @@ public function itAnchorsADeletionAtTheEndOfTheFile(): void new FileChange('file.xml', [], deletionAnchors: [2]), ); - self::assertTrue($scope->includes($this->violation(0, strlen($file->content) + 1, 1))); + $untilOffset = strlen($file->content); + + self::assertTrue($scope->includes($this->violation(0, $untilOffset + 1, 1))); + self::assertTrue($scope->includes($this->violation($untilOffset, $untilOffset, 2))); } private function violation(int $beginOffset, int $untilOffset, int $line): Violation diff --git a/tests/Unit/Runner/XmlFileProcessorPipelineTest.php b/tests/Unit/Runner/XmlFileProcessorPipelineTest.php index 4a117d5..504a10d 100644 --- a/tests/Unit/Runner/XmlFileProcessorPipelineTest.php +++ b/tests/Unit/Runner/XmlFileProcessorPipelineTest.php @@ -4,75 +4,159 @@ namespace DocbookCS\Tests\Unit\Runner; +use DocbookCS\Fix\Fix; +use DocbookCS\Fix\FixApplier; +use DocbookCS\Fix\FixPlan; +use DocbookCS\Fix\FixResult; +use DocbookCS\Fix\Fixer\AttributeOrderFixer; +use DocbookCS\Fix\Fixer\SimparaFixer; +use DocbookCS\Report\FileReport; use DocbookCS\Runner\EntityPreprocessor; use DocbookCS\Runner\RunMode; +use DocbookCS\Runner\RunScope; +use DocbookCS\Runner\ViolationScopeFilter; use DocbookCS\Runner\XmlFileProcessor; -use DocbookCS\Report\FileReport; +use DocbookCS\Runner\XmlFixRunner; +use DocbookCS\Runner\XmlSniffRunner; +use DocbookCS\Sniff\AbstractSniff; use DocbookCS\Sniff\AttributeOrderSniff; -use DocbookCS\Sniff\SniffInterface; +use DocbookCS\Sniff\Fixable; use DocbookCS\Source\File; -use PHPUnit\Framework\Attributes\CoversNothing; +use DocbookCS\Violation\SourceRange; +use DocbookCS\Violation\Violation; +use DocbookCS\Xml\XmlParser; +use DocbookCS\Tests\Support\XmlHelper; +use PHPUnit\Framework\Attributes\CoversClass; use PHPUnit\Framework\Attributes\Test; +use PHPUnit\Framework\Attributes\UsesClass; use PHPUnit\Framework\TestCase; -#[CoversNothing] +#[ + CoversClass(XmlFileProcessor::class), + // + UsesClass(AbstractSniff::class), + UsesClass(AttributeOrderFixer::class), + UsesClass(AttributeOrderSniff::class), + UsesClass(EntityPreprocessor::class), + UsesClass(File::class), + UsesClass(FileReport::class), + UsesClass(Fix::class), + UsesClass(FixApplier::class), + UsesClass(FixPlan::class), + UsesClass(FixResult::class), + UsesClass(RunMode::class), + UsesClass(RunScope::class), + UsesClass(SimparaFixer::class), + UsesClass(SourceRange::class), + UsesClass(Violation::class), + UsesClass(ViolationScopeFilter::class), + UsesClass(XmlFixRunner::class), + UsesClass(XmlParser::class), + UsesClass(XmlSniffRunner::class), +] final class XmlFileProcessorPipelineTest extends TestCase { + use XmlHelper; + #[Test] - public function itKeepsTheActualSourcePathInViolations(): void + public function itReportsXmlParseErrors(): void { - $workingDirectory = getcwd(); - self::assertIsString($workingDirectory); - - $filePath = tempnam($workingDirectory, 'docbook-cs-'); - self::assertIsString($filePath); - - try { - file_put_contents($filePath, ''); - - $report = $this->process( - $this->processor([new AttributeOrderSniff()]), - '', - $filePath, - ); - - self::assertCount(1, $report->getViolations()); - self::assertSame($filePath, $report->getViolations()[0]->filePath); - self::assertSame($filePath, $report->filePath); - } finally { - @unlink($filePath); - } + $file = new File('input.xml', ''); + $fileReport = new FileReport($file->path); + + $fixedFile = new XmlFileProcessor( + new XmlSniffRunner(RunMode::Sniff, []), + )->process($file, $fileReport, RunScope::fromFileAndFileChange($file, null)); + + self::assertNull($fixedFile); + self::assertSame(1, $fileReport->getFinalViolationCount()); + self::assertSame('DocbookCS.Internal', $fileReport->finalViolations[0]->sniffCode); } #[Test] - public function itAppliesFixesToTheOriginalSourceWhenEntitiesExpandBeforeTheViolation(): void + public function itStopsWhenAStaleFixCannotBeApplied(): void + { + $file = new File('input.xml', ''); + $fileReport = new FileReport($file->path); + $sniff = new class extends AbstractSniff implements Fixable { + public static function getCode(): string + { + return 'Test.StaleFix'; + } + + public static function getFixerClassName(): string + { + return SimparaFixer::class; + } + + public function process(\DOMDocument $document, File $file): array + { + return [$this->createViolation( + $file->path, + 'Stale fix.', + [ + new SourceRange(1, 0, 4, 'para'), + new SourceRange(1, 4, 8, 'para'), + ], + )]; + } + }; + + $fixedFile = new XmlFileProcessor( + new XmlSniffRunner(RunMode::Fix, [$sniff]), + )->process($file, $fileReport, RunScope::fromFileAndFileChange($file, null)); + + self::assertNull($fixedFile); + self::assertSame(1, $fileReport->getFoundViolationCount()); + self::assertSame(1, $fileReport->getFinalViolationCount()); + self::assertSame(1, $fileReport->fixingPasses); + self::assertFalse($fileReport->changed); + } + + #[Test] + public function itKeepsTheActualSourcePathInViolations(): void { - $filePath = tempnam(sys_get_temp_dir(), 'docbook-cs-'); - self::assertIsString($filePath); - $source = '&prefix;'; + $file = new File( + '/project/reference/file.xml', + '', + ); + $fileReport = new FileReport($file->path); + + new XmlFileProcessor( + new XmlSniffRunner(RunMode::Sniff, [new AttributeOrderSniff()]), + )->process($file, $fileReport, RunScope::fromFileAndFileChange($file, null)); - try { - file_put_contents($filePath, $source); + self::assertSame(1, $fileReport->getFinalViolationCount()); + self::assertSame($file->path, $fileReport->finalViolations[0]->filePath); + self::assertSame($file->path, $fileReport->filePath); + } - $processor = $this->processor( - [new AttributeOrderSniff(RunMode::Fix)], + #[Test] + public function itAppliesFixesToTheOriginalSourceWhenEntitiesExpandBeforeTheViolation(): void + { + $file = new File( + 'input.xml', + '&prefix;', + ); + $fileReport = new FileReport($file->path); + $fixedFile = new XmlFileProcessor( + new XmlSniffRunner( + RunMode::Fix, + [new AttributeOrderSniff()], new EntityPreprocessor([ 'prefix' => 'expanded-content-before-tag', ]), - ); - - $this->processFile($processor, $filePath); + ), + )->process($file, $fileReport, RunScope::fromFileAndFileChange($file, null)); - self::assertSame( - '&prefix;', - file_get_contents($filePath), - ); - } finally { - @unlink($filePath); - } + self::assertNotNull($fixedFile); + self::assertSame( + '&prefix;', + $fixedFile->content, + ); } - #[Test] // TODO: should be integration + #[Test] public function itHandlesEntitiesWithoutParseErrors(): void { $xml = $this->xml( @@ -82,74 +166,40 @@ public function itHandlesEntitiesWithoutParseErrors(): void ' ); - $processor = $this->processor([], new EntityPreprocessor([ - 'link.superglobals' => '', - 'php.ini' => '', - ])); - - $report = $this->process($processor, $xml); - - self::assertCount( - 0, - array_filter( - $report->getViolations(), - fn($v) => $v->sniffCode === 'DocbookCS.Internal' - ) + $file = new File('input.xml', $xml); + $fileReport = new FileReport($file->path); + new XmlFileProcessor( + new XmlSniffRunner(RunMode::Sniff, [], new EntityPreprocessor([ + 'link.superglobals' => '', + 'php.ini' => '', + ])), + )->process( + $file, + $fileReport, + RunScope::fromFileAndFileChange($file, null), ); + + self::assertFalse($fileReport->hasFinalViolations()); } - #[Test] // TODO: should be integration + #[Test] public function itUsesCustomPreprocessor(): void { - $processor = $this->processor([], new EntityPreprocessor([ - 'custom.entity' => '[X]', - ])); - $xml = $this->xml('&custom.entity;'); - - $report = $this->process($processor, $xml); - - self::assertCount( - 0, - array_filter( - $report->getViolations(), - fn($v) => $v->sniffCode === 'DocbookCS.Internal' - ) + $file = new File('input.xml', $xml); + $fileReport = new FileReport($file->path); + new XmlFileProcessor( + new XmlSniffRunner( + RunMode::Sniff, + [], + new EntityPreprocessor(['custom.entity' => '[X]']), + ), + )->process( + $file, + $fileReport, + RunScope::fromFileAndFileChange($file, null), ); - } - - private function process(XmlFileProcessor $processor, string $content, string $path = 'input.xml'): FileReport - { - return $processor->process(new File($path, $content))->fileReport; - } - private function processFile(XmlFileProcessor $processor, string $path): FileReport - { - $content = file_get_contents($path); - self::assertIsString($content); - - $result = $processor->process(new File($path, $content)); - if ($result->isModified()) { - file_put_contents($path, $result->fixedContent()); - } - - return $result->fileReport; - } - - private function xml(string $body): string - { - return << -$body -XML; - } - - /** @param list $sniffs */ - private function processor(array $sniffs = [], ?EntityPreprocessor $pre = null): XmlFileProcessor - { - return new XmlFileProcessor( - $sniffs, - $pre ?? new EntityPreprocessor([]) // always pass array - ); + self::assertFalse($fileReport->hasFinalViolations()); } } diff --git a/tests/Unit/Runner/XmlProcessingResultTest.php b/tests/Unit/Runner/XmlProcessingResultTest.php deleted file mode 100644 index e06f8e0..0000000 --- a/tests/Unit/Runner/XmlProcessingResultTest.php +++ /dev/null @@ -1,73 +0,0 @@ -'), - new File('input.xml', ''), - ); - - self::assertFalse($result->isModified()); - } - - #[Test] - public function itHasPendingFixesForModifiedContent(): void - { - $result = new XmlProcessingResult( - new FileReport('input.xml'), - new File('input.xml', ''), - new File('input.xml', ''), - ); - - self::assertTrue($result->isModified()); - } - - #[Test] - public function itThrowsWhenReadingFixedContentWithoutFixApplication(): void - { - $result = new XmlProcessingResult( - new FileReport('input.xml'), - new File('input.xml', ''), - new File('input.xml', ''), - ); - - $this->expectException(FixerException::class); - $this->expectExceptionMessageIsOrContains('Cannot read fixed content when no fix application was attempted.'); - - $result->fixedContent(); - } - - #[Test] - public function itReturnsFixedContentWhenFixApplicationExists(): void - { - $result = new XmlProcessingResult( - new FileReport('input.xml'), - new File('input.xml', ''), - new File('input.xml', ''), - ); - - self::assertSame('', $result->fixedContent()); - } -} diff --git a/tests/Unit/Runner/XmlFileProcessorTest.php b/tests/Unit/Runner/XmlSniffRunnerTest.php similarity index 74% rename from tests/Unit/Runner/XmlFileProcessorTest.php rename to tests/Unit/Runner/XmlSniffRunnerTest.php index 28f4b55..ad2a5cb 100644 --- a/tests/Unit/Runner/XmlFileProcessorTest.php +++ b/tests/Unit/Runner/XmlSniffRunnerTest.php @@ -8,13 +8,13 @@ use DocbookCS\Fix\Fixer\AttributeOrderFixer; use DocbookCS\Fix\FixerException; use DocbookCS\Report\FileReport; -use DocbookCS\Report\Report; use DocbookCS\Runner\EntityPreprocessor; +use DocbookCS\Runner\XmlFileProcessor; +use DocbookCS\Runner\XmlFixRunner; use DocbookCS\Runner\RunMode; use DocbookCS\Runner\RunScope; use DocbookCS\Runner\ViolationScopeFilter; -use DocbookCS\Runner\XmlFileProcessor; -use DocbookCS\Runner\XmlProcessingResult; +use DocbookCS\Runner\XmlSniffRunner; use DocbookCS\Sniff\Fixable; use DocbookCS\Sniff\SniffInterface; use DocbookCS\Source\File; @@ -22,6 +22,8 @@ use DocbookCS\Violation\Severity; use DocbookCS\Violation\SourceRange; use DocbookCS\Violation\Violation; +use DocbookCS\Xml\XmlParser; +use DocbookCS\Tests\Support\XmlHelper; use PHPUnit\Framework\Attributes\CoversClass; use PHPUnit\Framework\Attributes\Test; use PHPUnit\Framework\Attributes\UsesClass; @@ -30,10 +32,9 @@ #[ CoversClass(EntityPreprocessor::class), CoversClass(FileReport::class), - CoversClass(Report::class), CoversClass(Violation::class), CoversClass(ViolationScopeFilter::class), - CoversClass(XmlFileProcessor::class), + CoversClass(XmlSniffRunner::class), // UsesClass(AttributeOrderFixer::class), UsesClass(File::class), @@ -43,14 +44,18 @@ UsesClass(RunMode::class), UsesClass(RunScope::class), UsesClass(SourceRange::class), - UsesClass(XmlProcessingResult::class), + UsesClass(XmlFileProcessor::class), + UsesClass(XmlFixRunner::class), + UsesClass(XmlParser::class), ] -final class XmlFileProcessorTest extends TestCase +final class XmlSniffRunnerTest extends TestCase { + use XmlHelper; + #[Test] public function itReportsParseErrors(): void { - $report = $this->process($this->processor(), '', 'bad.xml'); + $report = $this->process($this->runner(), '', 'bad.xml'); $this->assertInternalError($report, 'XML parse error'); } @@ -59,7 +64,7 @@ public function itReportsParseErrors(): void public function itReportsParseErrorsOutsideChangedSourceRanges(): void { $report = $this->process( - $this->processor(), + $this->runner(), '', 'bad.xml', new FileChange('bad.xml', [99]), @@ -72,7 +77,7 @@ public function itReportsParseErrorsOutsideChangedSourceRanges(): void public function itStoresTheProvidedFilePathInFileReports(): void { $filePath = (getcwd() ?: '') . '/nonexistent/path/file.xml'; - $report = $this->process($this->processor(), '', $filePath); + $report = $this->process($this->runner(), '', $filePath); self::assertSame($filePath, $report->filePath); } @@ -82,9 +87,9 @@ public function itAcceptsValidXmlWithoutViolations(): void { $xml = $this->xml('ok'); - $report = $this->process($this->processor(), $xml); + $report = $this->process($this->runner(), $xml); - self::assertFalse($report->hasViolations()); + self::assertFalse($report->hasFinalViolations()); } #[Test] @@ -92,9 +97,9 @@ public function itReturnsZeroViolationsWithoutSniffs(): void { $xml = $this->xml('Hello'); - $report = $this->process($this->processor(), $xml); + $report = $this->process($this->runner(), $xml); - self::assertSame(0, $report->getViolationCount()); + self::assertSame(0, $report->getFinalViolationCount()); } #[Test] @@ -110,9 +115,9 @@ public function itReturnsAllViolationsWithoutDiffFiltering(): void ' ); - $report = $this->process($this->processor([$sniff]), $xml); + $report = $this->process($this->runner([$sniff]), $xml); - self::assertSame(2, $report->getViolationCount()); + self::assertSame(2, $report->getFinalViolationCount()); } #[Test] @@ -129,14 +134,14 @@ public function itFiltersViolationsByChangedLines(): void ); $report = $this->process( - $this->processor([$sniff]), + $this->runner([$sniff], mode: RunMode::Fix), $xml, 'f.xml', new FileChange('f.xml', [3]), ); - self::assertSame(1, $report->getViolationCount()); - self::assertSame(3, $report->violations[0]->rangeOne()->line); + self::assertSame(1, $report->getFinalViolationCount()); + self::assertSame(3, $report->finalViolations[0]->rangeOne()->line); } #[Test] @@ -155,13 +160,13 @@ public function itExpandsElementSpanForNestedChanges(): void ); $report = $this->process( - $this->processor([$sniff]), + $this->runner([$sniff]), $xml, 'x.xml', new FileChange('x.xml', [6]), ); - self::assertSame(1, $report->getViolationCount()); + self::assertSame(1, $report->getFinalViolationCount()); } #[Test] @@ -176,13 +181,13 @@ public function itDropsViolationsWhoseLineHasNoElement(): void ); $report = $this->process( - $this->processor([$sniff]), + $this->runner([$sniff]), $xml, 'x.xml', new FileChange('x.xml', [3]), ); - self::assertSame(0, $report->getViolationCount()); + self::assertSame(0, $report->getFinalViolationCount()); } #[Test] @@ -199,13 +204,13 @@ public function itMatchesChangesInElementOwnTextContent(): void ); $report = $this->process( - $this->processor([$sniff]), + $this->runner([$sniff]), $xml, 'x.xml', new FileChange('x.xml', [4]), ); - self::assertSame(1, $report->getViolationCount()); + self::assertSame(1, $report->getFinalViolationCount()); } #[Test] @@ -223,13 +228,13 @@ public function itBoundsChildSpanByNextSibling(): void ); $report = $this->process( - $this->processor([$sniff]), + $this->runner([$sniff]), $xml, 'x.xml', new FileChange('x.xml', [4]), ); - self::assertSame(1, $report->getViolationCount()); + self::assertSame(1, $report->getFinalViolationCount()); } #[Test] @@ -250,13 +255,13 @@ public function itIgnoresChangesInNonDirectDescendants(): void ); $report = $this->process( - $this->processor([$sniff]), + $this->runner([$sniff]), $xml, 'x.xml', new FileChange('x.xml', [6]), ); - self::assertSame(0, $report->getViolationCount()); + self::assertSame(0, $report->getFinalViolationCount()); } #[Test] @@ -272,13 +277,13 @@ public function itIgnoresChangesOutsideElementSpan(): void ); $report = $this->process( - $this->processor([$sniff]), + $this->runner([$sniff]), $xml, 'x.xml', new FileChange('x.xml', [7]), ); - self::assertSame(0, $report->getViolationCount()); + self::assertSame(0, $report->getFinalViolationCount()); } #[Test] @@ -295,23 +300,19 @@ public function itReportsNoViolationsInDiffModeWhenNoLinesWereAdded(): void ); $report = $this->process( - $this->processor([$sniff]), + $this->runner([$sniff]), $xml, 'f.xml', new FileChange('f.xml', []), ); - self::assertSame(0, $report->getViolationCount()); + self::assertSame(0, $report->getFinalViolationCount()); } #[Test] public function itDoesNotFixViolationsFromNonFixableSniffs(): void { - $sniff = new class (RunMode::Sniff) implements SniffInterface { - public function __construct(public RunMode $mode) - { - } - + $sniff = new /** @implements SniffInterface */ class implements SniffInterface { public static function getCode(): string { return 'Test.NonFixable'; @@ -335,19 +336,18 @@ public function setProperty(string $name, string $value): void } }; - $report = $this->process($this->processor([$sniff]), $this->xml('')); + $report = $this->process( + $this->runner([$sniff]), + $this->xml(''), + ); - self::assertSame(1, $report->getViolationCount()); + self::assertSame(1, $report->getFinalViolationCount()); } #[Test] public function itThrowsWhenFixableSniffReportsViolationWithoutContentInFixMode(): void { - $sniff = new class (RunMode::Fix) implements Fixable { - public function __construct(public RunMode $mode) - { - } - + $sniff = new /** @implements Fixable */ class implements Fixable { public static function getCode(): string { return 'Test.BrokenFixable'; @@ -379,20 +379,23 @@ public function setProperty(string $name, string $value): void $this->expectException(FixerException::class); $this->expectExceptionMessageIsOrContains('Fixers require affected source ranges with source content.'); - $this->process($this->processor([$sniff]), $this->xml('')); + $content = $this->xml(''); + $file = new File('input.xml', $content); + $fileReport = new FileReport($file->path); + new XmlFileProcessor($this->runner([$sniff], mode: RunMode::Fix))->process( + $file, + $fileReport, + RunScope::fromFileAndFileChange($file, null), + ); } /** @param list $lines */ private function sniff(array $lines): SniffInterface { - $sniff = new class (RunMode::Sniff) implements SniffInterface { + $sniff = new /** @implements SniffInterface */ class implements SniffInterface { /** @var list */ public array $lines = []; - public function __construct(public RunMode $mode) - { - } - public static function getCode(): string { return 'Test.Stub'; @@ -423,35 +426,35 @@ public function setProperty(string $name, string $value): void } private function process( - XmlFileProcessor $processor, + XmlSniffRunner $runner, string $content, string $path = 'input.xml', ?FileChange $fileChange = null, ): FileReport { - return $processor->process(new File($path, $content), $fileChange)->fileReport; - } + $file = new File($path, $content); + $fileReport = new FileReport($path); - /** @param list $sniffs */ - private function processor(array $sniffs = [], ?EntityPreprocessor $pre = null): XmlFileProcessor - { - return new XmlFileProcessor( - $sniffs, - $pre ?? new EntityPreprocessor([]) // always pass array + new XmlFileProcessor($runner)->process( + $file, + $fileReport, + RunScope::fromFileAndFileChange($file, $fileChange), ); + + return $fileReport; } - private function xml(string $body): string - { - return << -$body -XML; + /** @param list $sniffs */ + private function runner( + array $sniffs = [], + RunMode $mode = RunMode::Sniff, + ): XmlSniffRunner { + return new XmlSniffRunner($mode, $sniffs); } private function assertInternalError(FileReport $report, string $messagePart): void { - self::assertTrue($report->hasViolations()); - self::assertSame('DocbookCS.Internal', $report->getViolations()[0]->sniffCode); - self::assertStringContainsString($messagePart, $report->getViolations()[0]->message); + self::assertTrue($report->hasFinalViolations()); + self::assertSame('DocbookCS.Internal', $report->finalViolations[0]->sniffCode); + self::assertStringContainsString($messagePart, $report->finalViolations[0]->message); } } diff --git a/tests/Unit/Sniff/ExceptionNameSniffTest.php b/tests/Unit/Sniff/ExceptionNameSniffTest.php index 7b097fc..2a7f349 100644 --- a/tests/Unit/Sniff/ExceptionNameSniffTest.php +++ b/tests/Unit/Sniff/ExceptionNameSniffTest.php @@ -221,4 +221,18 @@ public function itIgnoresClassnamesInsideCommentsWhenMappingSource(): void self::assertSame('classname', $violations[0]->rangeOne()->content); self::assertSame('classname', $violations[0]->rangeTwo()->content); } + + #[Test] + public function itRejectsSourceThatDoesNotMatchTheParsedDocument(): void + { + $document = $this->createDocument('RuntimeException'); + + $this->expectException(\LogicException::class); + $this->expectExceptionMessageIs('Could not map classname violation to source content.'); + + new ExceptionNameSniff()->process( + $document, + new File('file.xml', 'OtherException'), + ); + } } diff --git a/tests/Unit/Sniff/SimparaSniffTest.php b/tests/Unit/Sniff/SimparaSniffTest.php index 7133099..69b4d96 100644 --- a/tests/Unit/Sniff/SimparaSniffTest.php +++ b/tests/Unit/Sniff/SimparaSniffTest.php @@ -214,4 +214,15 @@ public function itDoesNotFlagParaInFormalparaRegardlessOfCase(): void self::assertSame([], new SimparaSniff()->process($doc, new File('file.xml', $content))); } + + #[Test] + public function itRejectsSourceThatDoesNotMatchTheParsedDocument(): void + { + $document = $this->createDocument('Text'); + + $this->expectException(\LogicException::class); + $this->expectExceptionMessageIs('Could not map simpara violation to source content.'); + + new SimparaSniff()->process($document, new File('file.xml', '')); + } } diff --git a/tests/Unit/Sniff/WhitespaceConcernSniffsTest.php b/tests/Unit/Sniff/WhitespaceConcernSniffsTest.php index 6536700..bf87642 100644 --- a/tests/Unit/Sniff/WhitespaceConcernSniffsTest.php +++ b/tests/Unit/Sniff/WhitespaceConcernSniffsTest.php @@ -81,6 +81,22 @@ public function itReportsDisjointConcernsOnTheSameLine(): void ); } + #[Test] + public function itAllowsIndentationUsingOnlySpacesOrOnlyTabs(): void + { + foreach ([" ", "\t\t"] as $line) { + $content = "\n{$line}\n"; + + self::assertSame( + [], + new MixedIndentationSniff()->process( + $this->createDocument($content), + new File('file.xml', $content), + ), + ); + } + } + private function createDocument(string $xml): \DOMDocument { $document = new \DOMDocument(); diff --git a/tests/Unit/Source/FileTest.php b/tests/Unit/Source/FileTest.php index 7cca8e3..b5f6721 100644 --- a/tests/Unit/Source/FileTest.php +++ b/tests/Unit/Source/FileTest.php @@ -85,6 +85,38 @@ public function itReusesTheCurrentRevisionForUnchangedContent(): void self::assertSame($file, $file->withContent('')); } + #[Test] + public function itMasksNonElementMarkupWithoutChangingSourceOffsets(): void + { + $content = <<<'XML' + Declared">]> + + + CDATA]]> + Instruction?> + Source + + XML; + + $masked = new File('file.xml', $content)->contentWithNonElementMarkupMasked(); + + self::assertSame(strlen($content), strlen($masked)); + self::assertSame(strpos($content, 'Source'), strpos($masked, 'Source')); + self::assertStringNotContainsString('Declared', $masked); + self::assertStringNotContainsString('Commented', $masked); + self::assertStringNotContainsString('CDATA', $masked); + self::assertStringNotContainsString('Instruction', $masked); + } + + #[Test] + public function itMasksAnUnterminatedDeclarationThroughTheEndOfTheSource(): void + { + $source = 'contentWithNonElementMarkupMasked(); + + self::assertSame(str_repeat(' ', strlen($source)), $masked); + } + #[Test] public function itRejectsOffsetsOutsideTheSource(): void { diff --git a/tests/Unit/Xml/XmlParserTest.php b/tests/Unit/Xml/XmlParserTest.php new file mode 100644 index 0000000..074a260 --- /dev/null +++ b/tests/Unit/Xml/XmlParserTest.php @@ -0,0 +1,57 @@ +parseDocument(''); + + self::assertInstanceOf(\DOMDocument::class, $document); + self::assertSame('root', $document->documentElement?->localName); + } + + #[Test] + public function itParsesElements(): void + { + $element = new XmlParser()->parseElement(''); + + self::assertInstanceOf(\SimpleXMLElement::class, $element); + self::assertSame('root', $element->getName()); + } + + #[Test] + public function itReturnsParseErrorsForInvalidElements(): void + { + $error = new XmlParser()->parseElement(''); + + self::assertInstanceOf(\LibXMLError::class, $error); + self::assertStringContainsString('Premature end of data', $error->message); + } + + #[Test] + public function itReturnsTheFirstParseErrorAndRestoresTheErrorMode(): void + { + $previousUseErrors = libxml_use_internal_errors(false); + + try { + $error = new XmlParser()->parseDocument(''); + + self::assertInstanceOf(\LibXMLError::class, $error); + self::assertStringContainsString('Premature end of data', $error->message); + self::assertFalse(libxml_use_internal_errors()); + } finally { + libxml_use_internal_errors($previousUseErrors); + } + } +}