Skip to content

In Apply.semigroup test replace ListWrapper with Option - #2827

Merged
kailuowang merged 3 commits into
typelevel:masterfrom
kailuowang:improve-tests
May 4, 2019
Merged

In Apply.semigroup test replace ListWrapper with Option#2827
kailuowang merged 3 commits into
typelevel:masterfrom
kailuowang:improve-tests

Conversation

@kailuowang

Copy link
Copy Markdown
Contributor

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

def combine(a: F[A], b: F[A]): F[A] =
    f.map2(a, b)(sg.combine)

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 size a.size * b.size
When this combine is used in the combineAllOption test, which combine all items in a Vector[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.

@kailuowang kailuowang added this to the 2.0.0-RC1 milestone May 3, 2019
@kailuowang
kailuowang requested review from rossabaker and tpolecat May 3, 2019 16:53
@codecov-io

codecov-io commented May 3, 2019

Copy link
Copy Markdown

Codecov Report

Merging #2827 into master will decrease coverage by 0.29%.
The diff coverage is n/a.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
core/src/main/scala/cats/Apply.scala 75% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 422c989...bfcf98c. Read the comment docs.

rossabaker
rossabaker previously approved these changes May 3, 2019

@rossabaker rossabaker left a comment

Copy link
Copy Markdown
Member

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.

Copy link
Copy Markdown
Member

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

thanks!

@DavidGregory084

Copy link
Copy Markdown
Contributor

Thanks for looking into that @kailuowang!

@LukaJCB LukaJCB left a comment

Copy link
Copy Markdown
Member

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!

@kailuowang
kailuowang merged commit 9b6a6fd into typelevel:master May 4, 2019
@kailuowang kailuowang modified the milestones: 2.0.0-RC1, 2.0.0-M2 Jun 4, 2019
@kailuowang
kailuowang deleted the improve-tests branch April 22, 2020 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants