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

Add new TestCaseCovers sniff to control @covers tags in unit tests #178

Merged
merged 1 commit into from
Feb 1, 2022

Conversation

stronk7
Copy link
Member

@stronk7 stronk7 commented Jan 25, 2022

This new feature will be in charge of validating the presence and
correctness of PHPUnit coverage annotations/tags.

Restrictions:

  • It's only applied to Moodle 4.0 and up.
  • It's only applied to files under /tests directories.
  • It's only applied to files named _test.php.
  • It's only applied to classes named _test or _testcase.
  • it's only applied to methods named test_ .

Warnings implemented:

  • Warn if both @covers and @coversNothing information are missing.
  • Warn when there are contradictory mixed @coversNothing (at class) and
    @covers (at method) combinations.
  • Warn about redundant @coversNothing present both at class and method.

Errors implemented:

  • Error when there are contradictory @coversNothing and @covers in the
    same class or method.
  • Error if the @coversDefaultClass is used in method.
  • Error if the @coversNothing tag has any value.
  • Error if the @covers or @coversDefaultClass tags don't have any value.
  • Error if the @coversDefaultClass tag doesn't start with \ (FQCN).
  • Error if the @coversDefaultClass tag has :: (reserved for @covers).
  • Error if the @covers tag doesn't start with \ (FQCN) or :: (method).

96.67% line coverage achieved (soon will be 100%, another PR incoming).

Impact notes: See that, while adding @covers annotations to various
unit tests, it's also mandatory to add a description to them. That's
because the local_moodlecheck (in charge of examining phpdoc blocks)
accepts the absence of phpdoc blocks, but once they exist, the
description in mandatory. Not sure if we should relax that in
moodlecheck or no (being strict, I think it's correct).

@stronk7
Copy link
Member Author

stronk7 commented Jan 26, 2022

For the records, have run it against core with:

$ local/codechecker/phpcs/bin/phpcs --standard=moodle  --extensions=php --sniffs=moodle.PHPUnit.TestCaseCovers .

And:

  • run took 8 minutes.
  • 9329 warnings (99% of them about missing annotations.
  • 3 errors.

Ciao :-)

@stronk7 stronk7 force-pushed the new_testcase_covers_sniff branch from 4243cd5 to ac4a5c9 Compare January 26, 2022 07:13
This new feature will be in charge of validating the presence and
correctness of PHPUnit coverage annotations/tags.

Restrictions:
- It's only applied to Moodle 4.0 and up.
- It's only applied to files under /tests directories.
- It's only applied to files named `_test.php`.
- It's only applied to classes named `_test` or `_testcase`.
- it's only applied to methods named `test_ `.

Warnings implemented:
- Warn if both @Covers and @coversNothing information are missing.
- Warn when there are contradictory mixed @coversNothing (at class) and
  @Covers (at method) combinations.
- Warn about redundant @coversNothing present both at class and method.

Errors implemented:
- Error when there are contradictory @coversNothing and @Covers in the
  same class or method.
- Error if the @coversDefaultClass is used in method.
- Error if the @coversNothing tag has any value.
- Error if the @Covers or @coversDefaultClass tags don't have any value.
- Error if the @coversDefaultClass tag doesn't start with \ (FQCN).
- Error if the @coversDefaultClass tag has :: (reserved for @Covers).
- Error if the @Covers tag doesn't start with \ (FQCN) or :: (method).

96.67% line coverage achieved.

Impact notes: See that, while adding @Covers annotations to various
unit tests, it's also mandatory to add a description to them. That's
because the local_moodlecheck (in charge of examining phpdoc blocks)
accepts the absence of phpdoc blocks, but once they exist, the
description in mandatory. Not sure if we should relax that in
moodlecheck or no (being strict, I think it's correct).
@stronk7 stronk7 force-pushed the new_testcase_covers_sniff branch from ac4a5c9 to e96e959 Compare February 1, 2022 15:01
@stronk7
Copy link
Member Author

stronk7 commented Feb 1, 2022

Going ahead, after discussing it with the team (also was shared some days ago @ dev chat, just in case).

@stronk7 stronk7 merged commit 1bb6603 into moodlehq:master Feb 1, 2022
@stronk7 stronk7 deleted the new_testcase_covers_sniff branch February 1, 2022 15:29
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.

1 participant