-
-
Notifications
You must be signed in to change notification settings - Fork 374
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.
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
Exclude non-executable lines #892
Merged
sebastianbergmann
merged 10 commits into
sebastianbergmann:9.2
from
Slamdunk:exclude_non_executable_lines
Feb 16, 2022
Merged
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
5b5898f
Exclude non-executable lines
Slamdunk ea7bbe0
ExecutableLinesFindingVisitor: mirror xDebug/PCOV behavior
Slamdunk 4520d9d
Executable lines differ a lot between standard and path coverage: fil…
Slamdunk 564916a
Update test suite to reflect new executable LoC requirements
Slamdunk 91be05c
ExecutableLinesFindingVisitor: narrow match to actual xDebug/PCOV LoC…
Slamdunk 2f8bbbd
Minor optimization
Slamdunk 07df4ac
CachingUncoveredFileAnalyser: ensure next release gets fresh cache
Slamdunk 4a9b10f
Remove PHP 8.1 specific syntax from test suite
Slamdunk db5894a
Expand stub with an actual dead loc
Slamdunk ebe3dc9
Adapt HTML branch/path tests
Slamdunk File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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 wellThere was a problem hiding this comment.
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?There was a problem hiding this comment.
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
Faster?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I'll leave refactoring after the main solution has landed with all tests passing
I have no experience on Windows, but on Linux real cases give me negligible impact:
3 runs on Paratest, Standard SATA disk for cache
Before
After
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
There was a problem hiding this comment.
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
This PR
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.