-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
In Apply.semigroup test replace ListWrapper
with Option
#2827
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2827 +/- ##
=========================================
- Coverage 94.17% 93.87% -0.3%
=========================================
Files 368 368
Lines 6933 6955 +22
Branches 303 305 +2
=========================================
Hits 6529 6529
- Misses 404 426 +22
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.
Looks good, but see suggestion on scaladoc.
* This semigroup uses a product operation to combine `F`s, | ||
* if the `Apply[F].product` result in larger `F` e.g. when `F` is a `List`, | ||
* accumulative usage of this instance, such as `combineAll`, will result in | ||
* `F`s with exponentially increasing sizes. |
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.
Light copy editing (spans lines, so I couldn't GitHub it):
* This semigroup uses a product operation to combine `F`s.
* If the `Apply[F].product` results in larger `F` (i.e. when `F` is a `List`),
* accumulative usage of this instance, such as `combineAll`, will result in
* `F`s with exponentially increasing sizes.
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.
thanks!
Thanks for looking into that @kailuowang! |
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.
Thankfully we can eradicate one more source of flimsyness in the tests!
This is to address the root cause of #2648 and #2319, the target method of this test is the combine method provided by
Apply.semigroup
https://github.com/typelevel/cats/blob/master/core/src/main/scala/cats/Apply.scala#L223-L225
In the case of
F
being a list, since this is a product operation between the two lists, the result is a list of the sizea.size * b.size
When this
combine
is used in thecombineAllOption
test, which combine all items in aVector[ListWrapper[Int]]
, it exponentially increase the size of the accumulative result list. That leads to a list of millions of items and over limit GC pressure that breaks the build.I switch it to using
Option
instead.