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

Enable coverage and docs #3096

Merged
merged 6 commits into from
Oct 11, 2019

Conversation

DieBauer
Copy link
Contributor

@DieBauer DieBauer commented Oct 2, 2019

Addresses two points of #2347

Enable scoverage for 2.13 and enable scaladoc generation for 2.13.

One caveat with scoverage is that, for scala 2.13.1 (which Cats currently doesn't use), there are issues as described here scoverage/sbt-scoverage#295 and blocking an upgrade.

This might be decisive in dropping that commit from this PR, let me know.

LukaJCB
LukaJCB previously approved these changes Oct 2, 2019
@codecov-io
Copy link

codecov-io commented Oct 2, 2019

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3096      +/-   ##
==========================================
- Coverage   93.52%   93.17%   -0.36%     
==========================================
  Files         368      372       +4     
  Lines        6998     7179     +181     
  Branches      198      207       +9     
==========================================
+ Hits         6545     6689     +144     
- Misses        453      490      +37
Flag Coverage Δ
#scala_version_212 93.52% <ø> (?)
#scala_version_213 90.86% <ø> (?)
Impacted Files Coverage Δ
...e/src/main/scala-2.13+/cats/data/ZipLazyList.scala 77.77% <0%> (ø)
...src/main/scala-2.13+/cats/instances/lazyList.scala 100% <0%> (ø)
....13+/cats/kernel/instances/LazyListInstances.scala 100% <0%> (ø)
.../main/scala-2.13+/cats/data/NonEmptyLazyList.scala 60.86% <0%> (ø)
...in/scala/cats/data/AbstractNonEmptyInstances.scala 100% <0%> (+3.44%) ⬆️

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 ca591fc...3f98b90. Read the comment docs.

@kailuowang
Copy link
Contributor

As of now on travis we only run coverage on Scala 2.12 using this job

With this change we shall be able to delete this coverage dedicated job and run coverage for both 2.12 and 2.13 by changing the jvm tests job to do coverage as well. I.e. change the following
https://github.com/typelevel/cats/blob/master/.travis.yml#L47
to

      install: pip install --user codecov
      script: sbt ++$TRAVIS_SCALA_VERSION!  coverage buildJVM bench/test coverageReport && codecov

@DieBauer
Copy link
Contributor Author

DieBauer commented Oct 3, 2019

@kailuowang thanks for the remark, I forgot about travis.

I'm reading the PR (#2737) that added the 2.12 coverage job, and there it states that running coverage reports for multiple scala versions is 'undefined'.

So I'm not entirely sure if this is what we want to enable here. What are your thoughts?

Also, regarding publishing API docs for 2.12 and 2.13 does anything need to change? I'm not familiar with microsite. In my projects I'm doing +unidoc and move the /target/docs/scala-x to a server.

@kailuowang
Copy link
Contributor

My thought was enabling coverage on both Scala versions makes some sense. True we will only get one codecov report, but I believe we have coverage threshold check, which fails the build if the delta coverage falls below (on the other hand I haven't seen that in action for a while, so maybe we need to check the settings, not necessarily in this PR though). For those coverage checks it kind of makes sense to have them on both Scala versions.

Unfortunately our microsite is tied to only one Scala version. I propose we change it to Scala 2.13, although I am not 100% sure if sbt-microsite supports it yet.

@DieBauer
Copy link
Contributor Author

DieBauer commented Oct 8, 2019

Ok that makes sense. Not sure what the correct place would be in the travis job though

It seems that bench/test works on 2.13.0 as well. So the comment (https://github.com/typelevel/cats/blob/master/.travis.yml#L51) is not valid anymore and we can merge the jobs
with something like this?

    - &jvm_tests
      stage: test
      env: TEST="JVM tests"
      script: sbt ++$TRAVIS_SCALA_VERSION! buildJVM bench/test
      scala:
      - *scala_version_212
      - *scala_version_213

Where would coverage reporting then fit in && codecov? in this job, or still in the separate job that you mentioned?

@kailuowang
Copy link
Contributor

that's good news.
yeah, let's put the coverage in the jvm_tests jobs

@DieBauer DieBauer force-pushed the enable-coverage-and-docs branch 4 times, most recently from 3911911 to d247bf2 Compare October 9, 2019 08:28
Copy link
Contributor

@kailuowang kailuowang left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks for discovering the flag.
Just two nit comments

build.sbt Outdated Show resolved Hide resolved
build.sbt Outdated Show resolved Hide resolved
kailuowang
kailuowang previously approved these changes Oct 9, 2019
Copy link
Contributor

@kailuowang kailuowang left a comment

Choose a reason for hiding this comment

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

Thanks!

@DieBauer
Copy link
Contributor Author

Fixed merge conflict with master.

@kailuowang kailuowang merged commit c8b2f4e into typelevel:master Oct 11, 2019
@travisbrown travisbrown added this to the 2.1.0-RC1 milestone Nov 6, 2019
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