-
Notifications
You must be signed in to change notification settings - Fork 5
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
Improve code coverage and fix bug #55
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #55 +/- ##
=============================================
+ Coverage 98.98% 99.83% +0.84%
- Complexity 220 221 +1
=============================================
Files 30 30
Lines 592 593 +1
=============================================
+ Hits 586 592 +6
+ Misses 6 1 -5
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One small commend, and conflicts to resolve - then we're good to go for the next release!
$this->values = []; | ||
|
||
/* @var UsergroupAwareMultiValueItem $value */ | ||
foreach ($values as $value) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding in a loop is super inefficient when we could just assign the input array. To achieve 100% coverage, I'd rather have a test that just gets the value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forget what I said. I now realize that this fixes a bug.
This will increase code coverage to 100%.
There is also a fix implemented which led to not existing keywords and ordernumbers if added via the setAllxyz method.