From 11d2a73a65dc50a22a8bd8f1506667cc9954bdd7 Mon Sep 17 00:00:00 2001 From: Michel Dekker Date: Mon, 21 May 2018 20:49:45 +0200 Subject: [PATCH 1/2] * Fixed undefined index in merge() method * Removed code from merge() to yield correct merge results * Added test case for merging coverage files in reverse order (to prevent undefined index again) * Changed merge test cases to assert against the expected output from a single coverage run, instead of the currently "bugged" result. --- src/CodeCoverage.php | 24 +----------------------- tests/TestCase.php | 16 +++++++++++----- tests/tests/CodeCoverageTest.php | 15 +++++++++++++-- 3 files changed, 25 insertions(+), 30 deletions(-) diff --git a/src/CodeCoverage.php b/src/CodeCoverage.php index b5f3954bc..4380fa823 100644 --- a/src/CodeCoverage.php +++ b/src/CodeCoverage.php @@ -380,30 +380,8 @@ public function merge(self $that): void continue; } - if ((\count($lines) > 0) && (\count($this->data[$file]) > 0) && (\count($lines) != \count($this->data[$file]))) { - if (\count($lines) > \count($ours = $this->data[$file])) { - // More lines in the one being added in - $lines = \array_filter( - $lines, - function ($value, $key) use ($ours) { - return \array_key_exists($key, $ours); - }, - \ARRAY_FILTER_USE_BOTH - ); - } else { - // More lines in the one we currently have - $this->data[$file] = \array_filter( - $this->data[$file], - function ($value, $key) use ($lines) { - return \array_key_exists($key, $lines); - }, - \ARRAY_FILTER_USE_BOTH - ); - } - } - foreach ($lines as $line => $data) { - if ($data === null || $this->data[$file][$line] === null) { + if ($data === null || (array_key_exists($line, $this->data[$file]) && $this->data[$file][$line] === null)) { // if the line is marked as "dead code" in either, mark it as dead code in the merged result $this->data[$file][$line] = null; } elseif (!isset($this->data[$file][$line])) { diff --git a/tests/TestCase.php b/tests/TestCase.php index ee25360de..871ee473d 100644 --- a/tests/TestCase.php +++ b/tests/TestCase.php @@ -238,8 +238,12 @@ protected function getExpectedDataArrayForBankAccount() 0 => 'BankAccountTest::testBalanceIsInitiallyZero', 1 => 'BankAccountTest::testDepositWithdrawMoney' ], + 9 => null, 13 => [], + 14 => [], + 15 => [], 16 => [], + 18 => [], 22 => [ 0 => 'BankAccountTest::testBalanceCannotBecomeNegative2', 1 => 'BankAccountTest::testDepositWithdrawMoney' @@ -247,6 +251,7 @@ protected function getExpectedDataArrayForBankAccount() 24 => [ 0 => 'BankAccountTest::testDepositWithdrawMoney', ], + 25 => null, 29 => [ 0 => 'BankAccountTest::testBalanceCannotBecomeNegative', 1 => 'BankAccountTest::testDepositWithdrawMoney' @@ -254,17 +259,18 @@ protected function getExpectedDataArrayForBankAccount() 31 => [ 0 => 'BankAccountTest::testDepositWithdrawMoney' ], + 32 => null ] ]; } - protected function getExpectedDataArrayForBankAccount2() + protected function getExpectedDataArrayForBankAccountInReverseOrder() { return [ TEST_FILES_PATH . 'BankAccount.php' => [ 8 => [ - 0 => 'BankAccountTest::testBalanceIsInitiallyZero', - 1 => 'BankAccountTest::testDepositWithdrawMoney' + 0 => 'BankAccountTest::testDepositWithdrawMoney', + 1 => 'BankAccountTest::testBalanceIsInitiallyZero' ], 9 => null, 13 => [], @@ -281,8 +287,8 @@ protected function getExpectedDataArrayForBankAccount2() ], 25 => null, 29 => [ - 0 => 'BankAccountTest::testBalanceCannotBecomeNegative', - 1 => 'BankAccountTest::testDepositWithdrawMoney' + 0 => 'BankAccountTest::testDepositWithdrawMoney', + 1 => 'BankAccountTest::testBalanceCannotBecomeNegative' ], 31 => [ 0 => 'BankAccountTest::testDepositWithdrawMoney' diff --git a/tests/tests/CodeCoverageTest.php b/tests/tests/CodeCoverageTest.php index 806c7e9e1..2822f712c 100644 --- a/tests/tests/CodeCoverageTest.php +++ b/tests/tests/CodeCoverageTest.php @@ -215,7 +215,7 @@ public function testCollect() $coverage = $this->getCoverageForBankAccount(); $this->assertEquals( - $this->getExpectedDataArrayForBankAccount2(), + $this->getExpectedDataArrayForBankAccount(), $coverage->getData() ); @@ -241,6 +241,17 @@ public function testMerge() ); } + public function testMergeReverseOrder() + { + $coverage = $this->getCoverageForBankAccountForLastTwoTests(); + $coverage->merge($this->getCoverageForBankAccountForFirstTwoTests()); + + $this->assertEquals( + $this->getExpectedDataArrayForBankAccountInReverseOrder(), + $coverage->getData() + ); + } + public function testMerge2() { $coverage = new CodeCoverage( @@ -251,7 +262,7 @@ public function testMerge2() $coverage->merge($this->getCoverageForBankAccount()); $this->assertEquals( - $this->getExpectedDataArrayForBankAccount2(), + $this->getExpectedDataArrayForBankAccount(), $coverage->getData() ); } From 494376e812ca09857c65de4e377945b44a05203e Mon Sep 17 00:00:00 2001 From: Michel Dekker Date: Tue, 22 May 2018 02:00:44 +0200 Subject: [PATCH 2/2] Correctly merged code coverage --- src/CodeCoverage.php | 53 ++++++++++++++++++++++++++------ tests/tests/CodeCoverageTest.php | 3 ++ 2 files changed, 46 insertions(+), 10 deletions(-) diff --git a/src/CodeCoverage.php b/src/CodeCoverage.php index 4380fa823..df2be0886 100644 --- a/src/CodeCoverage.php +++ b/src/CodeCoverage.php @@ -380,17 +380,23 @@ public function merge(self $that): void continue; } - foreach ($lines as $line => $data) { - if ($data === null || (array_key_exists($line, $this->data[$file]) && $this->data[$file][$line] === null)) { - // if the line is marked as "dead code" in either, mark it as dead code in the merged result - $this->data[$file][$line] = null; - } elseif (!isset($this->data[$file][$line])) { - // if no data has been set in the current data, overwrite all - $this->data[$file][$line] = $data; - } else { - // otherwise merge data from both coverage files + // we should compare the lines if any of two contains data + $compareLineNumbers = \array_unique( + \array_merge( + \array_keys($this->data[$file]), + \array_keys($that->data[$file]) + ) + ); + + foreach ($compareLineNumbers as $line) { + $thatPriority = $this->getLinePriority($that->data[$file], $line); + $thisPriority = $this->getLinePriority($this->data[$file], $line); + + if ($thatPriority > $thisPriority) { + $this->data[$file][$line] = $that->data[$file][$line]; + } elseif ($thatPriority === $thisPriority && \is_array($this->data[$file][$line])) { $this->data[$file][$line] = \array_unique( - \array_merge($this->data[$file][$line], $data) + \array_merge($this->data[$file][$line], $that->data[$file][$line]) ); } } @@ -400,6 +406,33 @@ public function merge(self $that): void $this->report = null; } + /** + * Determine the priority for a line + * + * 1 = the line is not set + * 2 = the line has not been tested + * 3 = the line is dead code + * 4 = the line has been tested + * + * During a merge, a higher number is better. + * + * @param array $data + * @param int $line + * @return int + */ + protected function getLinePriority($data, $line) + { + if (!\array_key_exists($line, $data)) { + return 1; + } elseif (\is_array($data[$line]) && \count($data[$line]) === 0) { + return 2; + } elseif ($data[$line] === null) { + return 3; + } + + return 4; + } + public function setCacheTokens(bool $flag): void { $this->cacheTokens = $flag; diff --git a/tests/tests/CodeCoverageTest.php b/tests/tests/CodeCoverageTest.php index 2822f712c..14f2125d7 100644 --- a/tests/tests/CodeCoverageTest.php +++ b/tests/tests/CodeCoverageTest.php @@ -10,6 +10,9 @@ namespace SebastianBergmann\CodeCoverage; +require __DIR__ . '/../_files/BankAccount.php'; +require __DIR__ . '/../_files/BankAccountTest.php'; + use SebastianBergmann\CodeCoverage\Driver\Driver; use SebastianBergmann\CodeCoverage\Driver\PHPDBG; use SebastianBergmann\CodeCoverage\Driver\Xdebug;