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

Fixing Eq[Function1] in testsJS; break JS build to separate matrix build. #1671

Merged
merged 4 commits into from
May 16, 2017

Conversation

kailuowang
Copy link
Contributor

@kailuowang kailuowang commented May 16, 2017

This PR breaks out the scalajs builds to separate travis matrix build to cut down build time which was exceeding the 50 minute hard limit set by traivs.
see #1666 for further discussion.

@codecov-io
Copy link

codecov-io commented May 16, 2017

Codecov Report

Merging #1671 into master will increase coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1671      +/-   ##
==========================================
+ Coverage   93.38%   93.43%   +0.04%     
==========================================
  Files         241      241              
  Lines        4054     4068      +14     
  Branches      134      140       +6     
==========================================
+ Hits         3786     3801      +15     
+ Misses        268      267       -1
Impacted Files Coverage Δ
core/src/main/scala/cats/data/Kleisli.scala 92.64% <100%> (ø) ⬆️
laws/src/main/scala/cats/laws/discipline/Eq.scala 84.37% <100%> (ø) ⬆️
core/src/main/scala/cats/MonadError.scala 100% <0%> (ø) ⬆️
core/src/main/scala/cats/syntax/monadError.scala 100% <0%> (ø) ⬆️
...n/scala/cats/laws/discipline/MonadErrorTests.scala 100% <0%> (ø) ⬆️
core/src/main/scala/cats/data/Validated.scala 100% <0%> (ø) ⬆️
laws/src/main/scala/cats/laws/MonadErrorLaws.scala 100% <0%> (ø) ⬆️
core/src/main/scala/cats/syntax/either.scala 99.13% <0%> (+0.02%) ⬆️
core/src/main/scala/cats/data/EitherT.scala 96.52% <0%> (+0.09%) ⬆️
core/src/main/scala/cats/instances/either.scala 100% <0%> (+2.38%) ⬆️

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 cb949b8...4b23d0a. Read the comment docs.

@kailuowang kailuowang changed the title increase the number of arbitrary inputs in Eq[A=>B] to guard against … Fixing Eq[Function1] in testsJS; break JS build to separate matrix build. May 16, 2017
Copy link
Contributor

@ceedubs ceedubs left a comment

Choose a reason for hiding this comment

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

Thanks @kailuowang! One minor question about a blank test class, but otherwise this looks great to me.

@@ -0,0 +1,5 @@
package cats.laws.discipline
Copy link
Contributor

Choose a reason for hiding this comment

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

What's this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops that's an accident. Will remove after I get to a computer

@@ -20,18 +20,24 @@ export publish_cmd="publishLocal"
if [[ $TRAVIS_PULL_REQUEST == "false" && $TRAVIS_BRANCH == "master" && $(cat version.sbt) =~ "-SNAPSHOT" ]]; then
export publish_cmd="publish gitSnapshots publish"
# temporarily disable to stabilize travis
#if [[ $TRAVIS_SCALA_VERSION = "2.11.8" ]]; then
# export publish_cmd="$publish_cmd ghpagesPushSite"
#if [[ $TRAVIS_SCALA_VERSION = "2.11.11" ]]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

Kind of tangential to this PR, and it looks like the code is commented out anyway, but we may want to change this to something like if [[ $TRAVIS_SCALA_VERSION =~ ^2\.11\. ]]. My knowledge on bash regular expressions is a bit foggy, so this may not be quite right.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I verified that the code works.

@kailuowang
Copy link
Contributor Author

@ceedubs feedback addressed.

@ceedubs
Copy link
Contributor

ceedubs commented May 16, 2017

Thanks @kailuowang! This looks good to me -- just waiting for the build to finish.

It looks like we are only using 1 worker in the scalatest/scalacheck configuration. Do you remember whether we've tried to increase that to reduce build times?

@kailuowang
Copy link
Contributor Author

It looks like we are only using 1 worker in the scalatest/scalacheck configuration. Do you remember whether we've tried to increase that to reduce build times?

I don't, but maybe @BennyHill does?

@ghost
Copy link

ghost commented May 16, 2017

From memory:

On scala.js, you only have one thread, so setting more workers will have no difference.

On jvm, it may help, if you have lots of threads. But sbt tends to grab lots of threads first, and besides, on Travis-CI we do not have many threads to start with - and I believe we run tests in a separate JVM as well

So it still might be worth looking, but I would rather spend my time elsewhere

@ceedubs
Copy link
Contributor

ceedubs commented May 16, 2017

@BennyHill thanks for the input. Since it's really easy to try bumping the number of workers, I'll give it a try in a PR once this one is merged, but I won't get my hopes up :)

@ceedubs ceedubs merged commit 4b15dd3 into typelevel:master May 16, 2017
@kailuowang kailuowang modified the milestone: 1.0.0-MF May 18, 2017
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.

4 participants