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

[2.x] Add covers attribute #492

Merged
merged 20 commits into from
Mar 9, 2022

Conversation

danilopolani
Copy link
Contributor

@danilopolani danilopolani commented Mar 5, 2022

Q A
Bug fix? no
New feature? yes
Fixed tickets #32

This PR tries to implement the @covers annotation. Actually, following the issue sebastianbergmann/phpunit#4502 , I decided to implement it as a PHP Attribute since the annotations will be deprecated in PHPUnit 11.

I haven't tested this yet because I started working on it on Windows and 90% of the commands do not work, will come back on this in the next few days.

Because of the logic of Pest (where final user does not have a class), I think it's not possible to implement the @coversDefaultClass, but that's not a big deal since the other methods works just fine without it.

Usage

test('covers class')
    ->expect(true)->toBeTrue()
    ->coversClass(Foo::class, Bar::class); // Same as `@covers MyClass`

test('covers global function')
    ->expect(true)->toBeTrue()
    ->coversFunction('globalFunction', 'anotherFunction'); // Same as `@covers ::functionName`

test('covers nothing')
    ->expect(true)->toBeTrue()
    ->coversNothing(); // Same as `@coversNothing`

Todo

  • Move attributes above class and not method (only allowed on classes; @covers was allowed on methods but it does not make sense to mix different coverage targets in a test case class)
  • Check FQN when passing a class name string (when it's passed to the Attribute)
  • Lint
  • Add tests
  • Add smart "covers()" that accepts both classes and functions and checks if they exists (?)

@nunomaduro
Copy link
Member

@danilopolani Looking very interesting so far!

@danilopolani danilopolani marked this pull request as ready for review March 7, 2022 17:26
Copy link
Member

@owenvoke owenvoke left a comment

Choose a reason for hiding this comment

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

Quick few things, but looks great to me! Thanks for adding the tests too! 🥳

src/Factories/Attributes/Attribute.php Outdated Show resolved Hide resolved
src/Factories/TestCaseFactory.php Outdated Show resolved Hide resolved
src/PendingCalls/TestCall.php Show resolved Hide resolved
src/PendingCalls/TestCall.php Outdated Show resolved Hide resolved
@lukeraymonddowning
Copy link
Member

Brilliant work here 👌

@danilopolani
Copy link
Contributor Author

@owenvoke thank you for the review! At the end I've moved coversNothing to method annotations and I've left the "methods attribute" code generation as well, although right now not used, because it will be needed in the future.

Let me know if you prefer me to remove it because not needed or if it's fine :)

@nunomaduro nunomaduro merged commit cda4665 into pestphp:master Mar 9, 2022
@nunomaduro
Copy link
Member

Thank you for this! We may reach in the future, asking for docs about this. ✅

@danilopolani
Copy link
Contributor Author

Thank you for this! We may reach in the future, asking for docs about this. ✅

Thank you Nuno! No problem, if you want I can already work on the PR for the docs 👀

@danilopolani danilopolani deleted the feat/covers-attribute branch March 9, 2022 10:31
@nunomaduro
Copy link
Member

@danilopolani Can you check what static types errors we have on master, and pull request a fix? Or they are not related to your PR?

@danilopolani
Copy link
Contributor Author

@danilopolani Can you check what static types errors we have on master, and pull request a fix? Or they are not related to your PR?

Yep sorry, I forgot to run test:types but only ran lint and tests 🤦 Sent a new PR -> #498

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants