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

Increase test coverage of the following Rules #1457

Closed
wants to merge 1 commit into from

Conversation

dcorrea777
Copy link
Contributor

  • Base64
  • Base
  • Bsn
  • Contains
  • Executable
  • FilterVar
  • In
  • Ip
  • Isbn
  • NoWhiteSpace
  • Pesel
  • Phone
  • PolishIdCard
  • PortugueseNif
  • PublicDomainSuffix
  • StartsWith
  • Subset
  • Version
  • VideoUrl

- Base64
- Base
- Bsn
- Contains
- Executable
- FilterVar
- In
- Ip
- Isbn
- NoWhiteSpace
- Pesel
- Phone
- PolishIdCard
- PortugueseNif
- PublicDomainSuffix
- StartsWith
- Subset
- Version
- VideoUrl
Copy link

codecov bot commented Apr 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.41%. Comparing base (2ae1df1) to head (5084aae).

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #1457      +/-   ##
============================================
+ Coverage     96.23%   97.41%   +1.17%     
  Complexity      923      923              
============================================
  Files           199      199              
  Lines          2125     2125              
============================================
+ Hits           2045     2070      +25     
+ Misses           80       55      -25     

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

Copy link
Member

@henriquemoody henriquemoody left a comment

Choose a reason for hiding this comment

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

Happy to see you here, @dcorrea777!

I've made a few suggestions removing the ->evaluate(...). That's because when we keep it, it's not clear that the exception is throw on a constructor level, in fact, it gives the impression that the exception is throw in the evaluate() method.

tests/unit/Rules/BaseTest.php Show resolved Hide resolved
tests/unit/Rules/PhoneTest.php Show resolved Hide resolved
tests/unit/Rules/VideoUrlTest.php Show resolved Hide resolved
@dcorrea777
Copy link
Contributor Author

I was actually a bit unsure whether it was really necessary to call the evaluate method, but now it's clear to me. Thank for you reply @henriquemoody

@henriquemoody
Copy link
Member

The only thing I would change in your merge request is the commit message. The short message says, "Increase test coverage of the following Rules," but when you see that in the commit list, that doesn't make much sense because the following rules are not in the short message. In this case, I would just say, "Increase code coverage for some rules"

@henriquemoody
Copy link
Member

henriquemoody commented Apr 23, 2024

Merged by 719f12a

Thank you for contributing, @dcorrea777!! 🐼

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