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

Move unit test method Missing Docblock to new code #127

Merged
merged 2 commits into from
Mar 20, 2024

Conversation

andrewnicols
Copy link
Contributor

Either:

  • Yes, it should
  • Yes, but let's use a different Sniff response so it can be filtered and/or ignored in phpcs.xml
  • No, it should not

All are very easy to implement.

I've gone for the middle ground for now.

Copy link

codecov bot commented Mar 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.82%. Comparing base (6974134) to head (613266f).

❗ Current head 613266f differs from pull request most recent head 801e32a. Consider uploading reports for the commit 801e32a to get more accurate results

Additional details and impacted files
@@            Coverage Diff            @@
##               main     #127   +/-   ##
=========================================
  Coverage     97.81%   97.82%           
- Complexity      825      827    +2     
=========================================
  Files            36       36           
  Lines          2425     2432    +7     
=========================================
+ Hits           2372     2379    +7     
  Misses           53       53           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@andrewnicols andrewnicols force-pushed the unitTestMethodsSeparate branch 2 times, most recently from 825a63e to a5bf302 Compare March 20, 2024 05:55
@stronk7
Copy link
Member

stronk7 commented Mar 20, 2024

Ho,

I'm not sure about this one... I don't think that we have required/suggested to document all the unit tests (functions) in the past (although I've not been able to find any reference in one - or the opposite - direction).

Uhm... impact out there can be certainly huge, if they were being skipped previously...

Not sure, not sure...

@andrewnicols
Copy link
Contributor Author

Bah - I think I missed a line in the old version. The old coding style made my brain completely miss that line, and there really weren't any good tests for the old one.

So we didn't previously add the error for test_ methods.

Added a commit to this patchset to do match that behaviour.

This allows it to be ignored in phpcs.xml.
@andrewnicols andrewnicols force-pushed the unitTestMethodsSeparate branch from f83aff3 to 613266f Compare March 20, 2024 07:59
@andrewnicols andrewnicols force-pushed the unitTestMethodsSeparate branch from 613266f to 801e32a Compare March 20, 2024 08:09
Copy link
Member

@stronk7 stronk7 left a comment

Choose a reason for hiding this comment

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

That looks perfect. I'll squash the 2 commits together, as far as they are highly coupled.

@stronk7 stronk7 merged commit 380108f into moodlehq:main Mar 20, 2024
6 checks passed
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.

2 participants