Skip to content
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

Chore: refactor docblock parsing into a more specific DocBlock value object with an associated registry #3836

Merged
merged 30 commits into from
Sep 7, 2019

Conversation

Ocramius
Copy link
Contributor

@Ocramius Ocramius commented Sep 7, 2019

No description provided.

src/Annotation/Registry.php Outdated Show resolved Hide resolved
];
$recordedOffsets[$matches['name'] . '_constraint'] = $offset;
} catch (\PharIo\Version\Exception $e) {
/* @TODO this catch is currently not valid, see https://github.com/phar-io/version/issues/16 */
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@epdenouden epdenouden added the event/code-sprint/2019-09 PHPUnit Code Sprint: September 2019 label Sep 7, 2019
@codecov
Copy link

codecov bot commented Sep 7, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@544dcd7). Click here to learn what that means.
The diff coverage is 86.95%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #3836   +/-   ##
=========================================
  Coverage          ?   82.96%           
  Complexity        ?     3868           
=========================================
  Files             ?      153           
  Lines             ?    10266           
  Branches          ?        0           
=========================================
  Hits              ?     8517           
  Misses            ?     1749           
  Partials          ?        0
Impacted Files Coverage Δ Complexity Δ
src/Annotation/Registry.php 100% <100%> (ø) 8 <8> (?)
src/Util/Test.php 90.37% <100%> (ø) 165 <6> (?)
src/Framework/Exception/Exception.php 100% <100%> (ø) 6 <2> (?)
src/Framework/TestSuite.php 75.73% <100%> (ø) 114 <0> (?)
src/Annotation/DocBlock.php 82.5% <82.5%> (ø) 74 <74> (?)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 544dcd7...f2e0483. Read the comment docs.

@codecov
Copy link

codecov bot commented Sep 7, 2019

Codecov Report

Merging #3836 into master will increase coverage by 0.12%.
The diff coverage is 86.48%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master   #3836      +/-   ##
===========================================
+ Coverage     82.98%   83.1%   +0.12%     
- Complexity     3824    3853      +29     
===========================================
  Files           151     153       +2     
  Lines         10107   10213     +106     
===========================================
+ Hits           8387    8488     +101     
- Misses         1720    1725       +5
Impacted Files Coverage Δ Complexity Δ
src/Runner/TestSuiteSorter.php 100% <ø> (ø) 61 <0> (ø) ⬇️
src/Annotation/Registry.php 100% <100%> (ø) 8 <8> (?)
src/Framework/Exception/Exception.php 100% <100%> (ø) 6 <2> (ø) ⬇️
src/Framework/TestSuite.php 75.73% <100%> (+0.08%) 114 <0> (ø) ⬇️
src/Annotation/DocBlock.php 82.64% <82.64%> (ø) 75 <75> (?)
src/Util/Test.php 90.29% <95.31%> (+3.79%) 165 <6> (-54) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5f8c409...905eda3. Read the comment docs.

@sebastianbergmann
Copy link
Owner

Looks like we found a bug in @muglug's Psalm:

Notice: Undefined property: DOMComment::$tagName in phar:///github/workspace/tools/psalm/src/Psalm/ErrorBaseline.php on line 84

Fatal error: Uncaught Error: Call to undefined method DOMComment::getAttribute() in phar:///github/workspace/tools/psalm/src/Psalm/ErrorBaseline.php:85
Stack trace:
#0 phar:///github/workspace/tools/psalm/src/psalm.php(407): _HumbugBox0ece64d7e152\Psalm\ErrorBaseline::read(Object(_HumbugBox0ece64d7e152\Psalm\Internal\Provider\FileProvider), '.psalm/baseline...')
#1 phar:///github/workspace/tools/psalm/psalm(5): require_once('phar:///github/...')
#2 /github/workspace/tools/psalm(14): require('phar:///github/...')
#3 {main}
  thrown in phar:///github/workspace/tools/psalm/src/Psalm/ErrorBaseline.php on line 85

https://github.com/sebastianbergmann/phpunit/pull/3836/checks?check_run_id=215062082

@Ocramius
Copy link
Contributor Author

Ocramius commented Sep 7, 2019

Possibly baseline.xml containing a comment:

    <!-- see https://github.com/phar-io/version/issues/16 -->

I'll remove the comment, since the file is likely to be only used (and updated) by the tool.

…alue type

This is just demonstrating the feasibility of moving some docblock parsing
out of `PHPUnit\Util\Test`, so we can refactor and cache annotation operations
where applicable.
This was moved to the new `DocBlock` API.
… ctor

Instead, relying on either `ReflectionMethod` or `ReflectionClass` is more
than enough.
A TODO has been introduced, but needs phar-io/version#16
to be fixed first.
…et)Tests` parameters

These were supposed to be `TestCase` rather than more generic `Test` instances.
…d to overhead

Parsing requirements on docblocks seems to be repeated multiple times,
so we need to prevent this from being performed more than necessary.
The regular expressions around this are quite complex, and therefore there
is a performance impact (~2% total runtime on an IO-free test suite).
…s()`

This method is used very often (10k+ times in a small test suite), so it
needs to be quite efficient, if possible.
…ToBeCoveredOrUsed()`

Replaced by a variadic `array_merge()` call
…e#(get|set)Tests` parameters"

This reverts commit 71432bd.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
event/code-sprint/2019-09 PHPUnit Code Sprint: September 2019
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants