Skip to content

Exclude non-executable lines #892

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
Feb 16, 2022
Merged

Exclude non-executable lines #892

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
5b5898f
Exclude non-executable lines
Slamdunk Feb 9, 2022
ea7bbe0
ExecutableLinesFindingVisitor: mirror xDebug/PCOV behavior
Slamdunk Feb 9, 2022
4520d9d
Executable lines differ a lot between standard and path coverage: fil…
Slamdunk Feb 9, 2022
564916a
Update test suite to reflect new executable LoC requirements
Slamdunk Feb 9, 2022
91be05c
ExecutableLinesFindingVisitor: narrow match to actual xDebug/PCOV LoC…
Slamdunk Feb 10, 2022
2f8bbbd
Minor optimization
Slamdunk Feb 10, 2022
07df4ac
CachingUncoveredFileAnalyser: ensure next release gets fresh cache
Slamdunk Feb 10, 2022
4a9b10f
Remove PHP 8.1 specific syntax from test suite
Slamdunk Feb 10, 2022
db5894a
Expand stub with an actual dead loc
Slamdunk Feb 11, 2022
ebe3dc9
Adapt HTML branch/path tests
Slamdunk Feb 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions src/CodeCoverage.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,8 @@ public function append(RawCodeCoverageData $rawData, $id = null, bool $append =

$this->applyFilter($rawData);

$this->applyExecutableLinesFilter($rawData);

if ($this->useAnnotationsForIgnoringCode) {
$this->applyIgnoredLinesFilter($rawData);
}
Expand Down Expand Up @@ -484,6 +486,16 @@ private function applyFilter(RawCodeCoverageData $data): void
}
}

private function applyExecutableLinesFilter(RawCodeCoverageData $data): void
{
foreach (array_keys($data->lineCoverage()) as $filename) {
$data->keepCoverageDataOnlyForLines(
$filename,
$this->uncoveredFileAnalyser()->executableLinesIn($filename)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be moved out of uncoveredFileAnalyser if it's going to be called for covered files as well

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there will also a performance impact from calling executableLinesIn so much that it might be a good idea to try and mitigate - if someone has caching enabled, then that will read from file each time but it might be a good idea to add a memory cache somewhere? Reads on Windows particularly are notoriously slow, and I think this would do an additional read per source file per test if my code-skim is correct?

Copy link
Contributor

@dvdoug dvdoug Feb 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For non-cached version:

Current

    public function enterNode(Node $node): void
    {
        if (!$this->isExecutable($node)) {
            return;
        }

        $this->executableLines[] = $node->getStartLine();
    }

    /**
     * @psalm-return list<int>
     */
    public function executableLines(): array
    {
        $executableLines = array_unique($this->executableLines);

        sort($executableLines);

        return $executableLines;
    }

Faster?



    public function enterNode(Node $node): void
    {
        if (!$this->isExecutable($node)) {
            return;
        }

        $this->executableLines[$node->getStartLine()] = $node->getStartLine();
    }

    /**
     * @psalm-return list<int>
     */
    public function executableLines(): array
    {
        return $executableLines;
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be moved out of uncoveredFileAnalyser if it's going to be called for covered files as well

Sure, I'll leave refactoring after the main solution has landed with all tests passing

I think there will also a performance impact from calling executableLinesIn so much

I have no experience on Windows, but on Linux real cases give me negligible impact:

Files: 906, Methods: 4402, Lines: 33708
Tests: 2989, Assertions: 19314, Skipped: 22

3 runs on Paratest, Standard SATA disk for cache

Before

Time: 02:02.831, Memory: 200.54 MB
Generating code coverage report ... done [00:06.284]

Time: 02:03.884, Memory: 198.84 MB
Generating code coverage report ... done [00:05.837]

Time: 02:02.206, Memory: 200.58 MB
Generating code coverage report ... done [00:05.936]

After

Time: 02:04.346, Memory: 176.52 MB
Generating code coverage report ... done [00:05.902]

Time: 02:06.492, Memory: 176.50 MB
Generating code coverage report ... done [00:05.635]

Time: 02:05.663, Memory: 176.52 MB
Generating code coverage report ... done [00:05.731]

$this->executableLines[$node->getStartLine()] = $node->getStartLine();

I thought this too: be aware that we need to version the cache, otherwise new release will break user reports if run onto old caches

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's an interesting post by an MS engineer at microsoft/WSL#873 (comment) in the context of WSL that compares/contrasts performance of the filesystem in Linux vs Windows. Suffice to say it's real

Windows 11, PHP 8.1.2, XDebug 3.1.3, running tests for this repo

9.2 branch

Time: 00:19.726, Memory: 360.00 MB

Time: 00:21.713, Memory: 360.00 MB

Time: 00:23.309, Memory: 360.00 MB

This PR

Time: 00:53.541, Memory: 374.00 MB

Time: 00:44.610, Memory: 374.00 MB

Time: 00:48.638, Memory: 374.00 MB

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the pointer, I'll have a look (hopefully tomorrow morning, otherwise over the weekend).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

I think the uncovered file analyser not having a memory cache is OK because it's normally only called once at the end of the entire test suite run just before generating the report, unlike the analyser for covered files that's called after every test. It's just that here, the relevant function is now being called from a very different context...

Copy link
Owner

@sebastianbergmann sebastianbergmann Feb 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed 3a3285f just now. It should not cause any harm, apart from producing a merge conflict for this PR.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thought about this some more: the idea to separate CoveredFileAnalyzer and UncoveredFileAnalyzer from each other was because the former was required after each test and the latter only once at the end. Now we also need the data from UncoveredFileAnalyzer after each test (right?). If that is really the case, then we should merge the two into FileAnalyzer.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have reverted 3a3285f (so that this PR no longer has conflicts). I think we should separate caching refactorings/improvements from the topic of this PR.

);
}
}

private function applyIgnoredLinesFilter(RawCodeCoverageData $data): void
{
foreach (array_keys($data->lineCoverage()) as $filename) {
Expand Down
27 changes: 27 additions & 0 deletions tests/tests/CodeCoverageTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,33 @@ public function testWhitelistFiltering(): void
$this->assertNotContains(TEST_FILES_PATH . 'CoverageClassTest.php', $this->coverage->getData()->coveredFiles());
}

public function testExcludeNonExecutableLines(): void
{
$this->coverage->filter()->includeFile(TEST_FILES_PATH . 'BankAccount.php');

$data = RawCodeCoverageData::fromXdebugWithoutPathCoverage([
TEST_FILES_PATH . 'BankAccount.php' => array_fill(1, 100, -1),
]);

$this->coverage->append($data, 'A test', true);

$expectedLineCoverage = [
TEST_FILES_PATH . 'BankAccount.php' => [
8 => [],
13 => [],
14 => [],
15 => [],
16 => [],
22 => [],
24 => [],
29 => [],
31 => [],
],
];

$this->assertEquals($expectedLineCoverage, $this->coverage->getData()->lineCoverage());
}

public function testMerge(): void
{
$coverage = $this->getLineCoverageForBankAccountForFirstTwoTests();
Expand Down