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

2.11.1 #97

Closed
wants to merge 15 commits into
base: master
from

Conversation

7 participants
@folone
Contributor

folone commented Apr 29, 2014

Re #95.
Here's some work on cross-compiling this codebase to scala 2.11.0.
The main thing here is migrating tests from specs to scalatest.
Some things to note are:

  • util-eval/test fails for 2.11.0
  • there are four ignored test cases in the util-zk (1, 2, 3, 4)

/cc @mosesn @bmdhacks

folone and others added some commits Apr 25, 2014

@mosesn

View changes

Show outdated Hide outdated project/Build.scala Outdated
@@ -103,13 +110,6 @@ object Util extends Build {
libraryDependencies ++= Seq(
"com.twitter.common" % "objectsize" % "0.0.7" % "test"
),
testOptions in Test <<= scalaVersion map {

This comment has been minimized.

@mosesn

mosesn Apr 29, 2014

Contributor

is this bug fixed in 0.13.2?

@mosesn

mosesn Apr 29, 2014

Contributor

is this bug fixed in 0.13.2?

This comment has been minimized.

@folone

folone Apr 30, 2014

Contributor

From the description of the bug, I've got that this is the problem with specs. Now that we're not using it, I figured this could be removed. Also, MonitorSpec seems to be working just fine.

@folone

folone Apr 30, 2014

Contributor

From the description of the bug, I've got that this is the problem with specs. Now that we're not using it, I figured this could be removed. Also, MonitorSpec seems to be working just fine.

@mosesn

View changes

Show outdated Hide outdated util-core/src/main/scala/com/twitter/util/BatchExecutor.scala Outdated
@mosesn

View changes

Show outdated Hide outdated util-core/src/main/scala/com/twitter/util/BoundedStack.scala Outdated
@mosesn

View changes

Show outdated Hide outdated util-core/src/main/scala/com/twitter/util/Promise.scala Outdated
@mosesn

View changes

Show outdated Hide outdated util-core/src/main/scala/com/twitter/util/RingBuffer.scala Outdated
@mosesn

View changes

Show outdated Hide outdated util-eval/src/main/scala/com/twitter/util/Eval.scala Outdated
@mosesn

View changes

Show outdated Hide outdated util-logging/src/main/scala/com/twitter/logging/ThrottledHandler.scala Outdated
@mosesn

View changes

Show outdated Hide outdated util-logging/src/main/scala/com/twitter/logging/ThrottledHandler.scala Outdated
@mosesn

View changes

Show outdated Hide outdated util-reflect/src/main/scala/com/twitter/util/reflect/Proxy.scala Outdated
@mosesn

View changes

Show outdated Hide outdated util-reflect/src/test/scala/com/twitter/util/ProxySpec.scala Outdated
@mosesn

View changes

Show outdated Hide outdated util-reflect/src/test/scala/com/twitter/util/ProxySpec.scala Outdated
@mosesn

View changes

Show outdated Hide outdated util-thrift/src/main/scala/com/twitter/util/ThriftCodec.scala Outdated
@mosesn

View changes

Show outdated Hide outdated util-zk/src/main/scala/com/twitter/zk/coordination/ZkAsyncSemaphore.scala Outdated
@mosesn

This comment has been minimized.

Show comment
Hide comment
@mosesn

mosesn Apr 29, 2014

Contributor

Looks generally good. A few things before we merge it in:

  1. We can't use any feature that was introduced after 2.9.2 until we drop support for scala 2.9.2. It will take a little while to do this, since Twitter still uses 2.9.2 in some places.
  2. Why are the util-zk tests broken? How can we help you fix them?
  3. All of the tests should have the @RunWith[JUnitRunner] annotation.
  4. We've historically called tests that were written with specs XSpec.scala and ones written with scalatest XTest.scala, so could you rename all of the tests that now use ScalaTest to end with Test.scala instead of Spec.scala?
  5. There are a few places where your word find/replace were a little overzealous and renamed variable names or comments.
Contributor

mosesn commented Apr 29, 2014

Looks generally good. A few things before we merge it in:

  1. We can't use any feature that was introduced after 2.9.2 until we drop support for scala 2.9.2. It will take a little while to do this, since Twitter still uses 2.9.2 in some places.
  2. Why are the util-zk tests broken? How can we help you fix them?
  3. All of the tests should have the @RunWith[JUnitRunner] annotation.
  4. We've historically called tests that were written with specs XSpec.scala and ones written with scalatest XTest.scala, so could you rename all of the tests that now use ScalaTest to end with Test.scala instead of Spec.scala?
  5. There are a few places where your word find/replace were a little overzealous and renamed variable names or comments.
@mosesn

View changes

Show outdated Hide outdated project/Build.scala Outdated
@folone

This comment has been minimized.

Show comment
Hide comment
@folone

folone Apr 30, 2014

Contributor

I'll address the issues and will let you know when I'm done.
Concerning the util-zk tests, it seems like they use jmock testing cycle, which I failed to reproduce in those places. Will see if JMockCycle can help there.

Contributor

folone commented Apr 30, 2014

I'll address the issues and will let you know when I'm done.
Concerning the util-zk tests, it seems like they use jmock testing cycle, which I failed to reproduce in those places. Will see if JMockCycle can help there.

@mosesn

This comment has been minimized.

Show comment
Hide comment
@mosesn

mosesn Apr 30, 2014

Contributor

Hmm, maybe it would be simpler to just keep everything on mockito and rephrase the test slightly?

Contributor

mosesn commented Apr 30, 2014

Hmm, maybe it would be simpler to just keep everything on mockito and rephrase the test slightly?

@folone

This comment has been minimized.

Show comment
Hide comment
@folone

folone May 3, 2014

Contributor

@mosesn Thanks for your comments, I've addressed them in folone@674b9f7. There's one particular difference between 2.9 and 2.11 that causes code duplication in util-collection: collection wrappers have been moved from scala.collection.JavaConversions to collection.convert.Wrappers (both work in 2.10 with first being deprecated). Therefore I used this trick with scala-2.11, scala-2.10 and scala-2.9.2 directories under util-collection/src/main.

Contributor

folone commented May 3, 2014

@mosesn Thanks for your comments, I've addressed them in folone@674b9f7. There's one particular difference between 2.9 and 2.11 that causes code duplication in util-collection: collection wrappers have been moved from scala.collection.JavaConversions to collection.convert.Wrappers (both work in 2.10 with first being deprecated). Therefore I used this trick with scala-2.11, scala-2.10 and scala-2.9.2 directories under util-collection/src/main.

Address code review.
- *Spec -> *Test
- RunWith(classOf[JUnitRunner])
- Add 2.9.2 to cross-compile targets
@mosesn

This comment has been minimized.

Show comment
Hide comment
@mosesn

mosesn May 4, 2014

Contributor

Instead of the different source directories, can we just use JavaConverters instead of JavaConversions?

Were you able to fix the util-zk problem?

Contributor

mosesn commented May 4, 2014

Instead of the different source directories, can we just use JavaConverters instead of JavaConversions?

Were you able to fix the util-zk problem?

@folone

This comment has been minimized.

Show comment
Hide comment
@folone

folone May 5, 2014

Contributor

Will have a look, thanks for pointing out. Haven't gotten to util-zk yet.

Contributor

folone commented May 5, 2014

Will have a look, thanks for pointing out. Haven't gotten to util-zk yet.

@bmdhacks

This comment has been minimized.

Show comment
Hide comment
@bmdhacks

bmdhacks May 22, 2014

Contributor

Awesome, I really want to get this in, I'll look at it today.

Contributor

bmdhacks commented May 22, 2014

Awesome, I really want to get this in, I'll look at it today.

@bmdhacks

This comment has been minimized.

Show comment
Hide comment
@bmdhacks

bmdhacks May 23, 2014

Contributor

I'm shepherding this through our internal submit process. Can't tell you how awesome this contribution is.

Contributor

bmdhacks commented May 23, 2014

I'm shepherding this through our internal submit process. Can't tell you how awesome this contribution is.

@folone

This comment has been minimized.

Show comment
Hide comment
@folone

folone May 23, 2014

Contributor

Awesome, let me know if you need me to make any changes.

Contributor

folone commented May 23, 2014

Awesome, let me know if you need me to make any changes.

@bmdhacks

This comment has been minimized.

Show comment
Hide comment
@bmdhacks

bmdhacks May 28, 2014

Contributor

Util master is a bit ahead of this patch. I've moved it forward but I'm running into trouble with com.twitter.logging.TestLogging since it uses specs as part of its API. Technically this means that removing specs is an API breaking change. Not sure what to do about that.

Contributor

bmdhacks commented May 28, 2014

Util master is a bit ahead of this patch. I've moved it forward but I'm running into trouble with com.twitter.logging.TestLogging since it uses specs as part of its API. Technically this means that removing specs is an API breaking change. Not sure what to do about that.

@folone

This comment has been minimized.

Show comment
Hide comment
@folone

folone Jun 2, 2014

Contributor

Yeah, I completely removed specs and updated TestLogging to expose scalatest API instead.

Contributor

folone commented Jun 2, 2014

Yeah, I completely removed specs and updated TestLogging to expose scalatest API instead.

@bajohns bajohns referenced this pull request Jun 9, 2014

Closed

Scala 2.11 support #129

@folone folone changed the title from 2.11.0 to 2.11.1 Jun 18, 2014

Update the build:
* Add -Xfuture flag to simplify going forward in the future
* Update scala version (2.11.0 -> 2.11.1)
* Update sbt version (0.13.2 -> 0.13.5).
@adamdecaf

This comment has been minimized.

Show comment
Hide comment
@adamdecaf

adamdecaf Jul 1, 2014

I'm looking to get this up to 2.11. How's it coming along? Anything I can help with?

adamdecaf commented Jul 1, 2014

I'm looking to get this up to 2.11. How's it coming along? Anything I can help with?

@mosesn

This comment has been minimized.

Show comment
Hide comment
@mosesn

mosesn Jul 1, 2014

Contributor

@adamdecaf this is basically blocked on me at this point. @bmdhacks has mostly wrestled it through CI, and I'm going to take it over the finish line. I'm going to try to get it in this week.

If you're interested in helping with the rest of the stack, we'd appreciate it if you took a stab at pieces of finagle that are still using specs.

Contributor

mosesn commented Jul 1, 2014

@adamdecaf this is basically blocked on me at this point. @bmdhacks has mostly wrestled it through CI, and I'm going to take it over the finish line. I'm going to try to get it in this week.

If you're interested in helping with the rest of the stack, we'd appreciate it if you took a stab at pieces of finagle that are still using specs.

@mosesn

This comment has been minimized.

Show comment
Hide comment
@mosesn

mosesn Jul 2, 2014

Contributor

Glorious news! This has been merged internally. We'll close this ticket after the change has been merged back into github.

If you have questions about how to move forward with 2.11 compat, I'll be your point of contact for now, and @bmdhacks will return to being the 2.11 guru when he's back from his honeymoon.

@folone your name will be sung for innumerable generations. If you ever find yourself in SF or NYC, hit us up. You are owed many beers.

Contributor

mosesn commented Jul 2, 2014

Glorious news! This has been merged internally. We'll close this ticket after the change has been merged back into github.

If you have questions about how to move forward with 2.11 compat, I'll be your point of contact for now, and @bmdhacks will return to being the 2.11 guru when he's back from his honeymoon.

@folone your name will be sung for innumerable generations. If you ever find yourself in SF or NYC, hit us up. You are owed many beers.

@folone

This comment has been minimized.

Show comment
Hide comment
@folone

folone Jul 2, 2014

Contributor

Looking forward to having util and finagle release for 2.11!
yess

Contributor

folone commented Jul 2, 2014

Looking forward to having util and finagle release for 2.11!
yess

@joegaudet

This comment has been minimized.

Show comment
Hide comment
@joegaudet

joegaudet commented Jul 9, 2014

+1

@electricmonk

This comment has been minimized.

Show comment
Hide comment
@electricmonk

electricmonk Jul 9, 2014

+1

On Wed, Jul 9, 2014 at 11:33 AM, Joe Gaudet notifications@github.com
wrote:

+1

Reply to this email directly or view it on GitHub:
#97 (comment)

electricmonk commented Jul 9, 2014

+1

On Wed, Jul 9, 2014 at 11:33 AM, Joe Gaudet notifications@github.com
wrote:

+1

Reply to this email directly or view it on GitHub:
#97 (comment)

folone added a commit that referenced this pull request Jul 16, 2014

[split] util-* Convert all tests in util to scalatest
Problem

specs is deprecated and throws a fair number of NullPointerExceptions in scala 2.10
Also all of our tests are disabled in scala 2.10 for sbt

Solution

Port all our tests to scalatest
This is based off of this PR: #97

Result

Tests pass

RB_ID=397681
@mosesn

This comment has been minimized.

Show comment
Hide comment
@mosesn

mosesn Aug 8, 2014

Contributor

This is in master, but we had a small hiccup with figuring out what to do with util-eval, so we were still unable to publish 2.11. Sorry! We're working on a solution right now. 😬

In the mean time, I'm going to close this PR. Thanks for the contribution!

Contributor

mosesn commented Aug 8, 2014

This is in master, but we had a small hiccup with figuring out what to do with util-eval, so we were still unable to publish 2.11. Sorry! We're working on a solution right now. 😬

In the mean time, I'm going to close this PR. Thanks for the contribution!

@mosesn mosesn closed this Aug 8, 2014

@folone

This comment has been minimized.

Show comment
Hide comment
@folone

folone Aug 8, 2014

Contributor

@mosesn So once you folks figure out util-eval issues, it'll be possible to just add 2.11 to cross-build targets, and everything would just work™, right?

Contributor

folone commented Aug 8, 2014

@mosesn So once you folks figure out util-eval issues, it'll be possible to just add 2.11 to cross-build targets, and everything would just work™, right?

@mosesn

This comment has been minimized.

Show comment
Hide comment
@mosesn

mosesn Aug 8, 2014

Contributor

Yep!

Contributor

mosesn commented Aug 8, 2014

Yep!

@folone

This comment has been minimized.

Show comment
Hide comment
@folone

folone Aug 8, 2014

Contributor

Sounds great! Thanks a lot! Do you have any kind of estimates on when to expect 2.11 version?

Contributor

folone commented Aug 8, 2014

Sounds great! Thanks a lot! Do you have any kind of estimates on when to expect 2.11 version?

@mosesn

This comment has been minimized.

Show comment
Hide comment
@mosesn

mosesn Aug 8, 2014

Contributor

Nothing hard yet, but my guess would be in a couple of weeks.

Contributor

mosesn commented Aug 8, 2014

Nothing hard yet, but my guess would be in a couple of weeks.

@folone

This comment has been minimized.

Show comment
Hide comment
@folone

folone Aug 10, 2014

Contributor

Great, thanks!

Contributor

folone commented Aug 10, 2014

Great, thanks!

@jedesah

This comment has been minimized.

Show comment
Hide comment
@jedesah

jedesah Aug 22, 2014

@mosesn What's the new ETA on this?

jedesah commented Aug 22, 2014

@mosesn What's the new ETA on this?

@mosesn

This comment has been minimized.

Show comment
Hide comment
@mosesn

mosesn Aug 22, 2014

Contributor

@jedesah we're still on track. I'll let you know if something goes wrong. Thanks for checking in!

Contributor

mosesn commented Aug 22, 2014

@jedesah we're still on track. I'll let you know if something goes wrong. Thanks for checking in!

@mosesn

This comment has been minimized.

Show comment
Hide comment
@mosesn

mosesn Aug 23, 2014

Contributor

It has been closed and released from sonatype. It should be synced to maven central shortly.

Contributor

mosesn commented Aug 23, 2014

It has been closed and released from sonatype. It should be synced to maven central shortly.

@mosesn

This comment has been minimized.

Show comment
Hide comment
@mosesn

mosesn Aug 23, 2014

Contributor

It's there! (๑>ᴗ<๑) big shoutout to @folone!

Contributor

mosesn commented Aug 23, 2014

It's there! (๑>ᴗ<๑) big shoutout to @folone!

@folone

This comment has been minimized.

Show comment
Hide comment
@folone

folone Aug 23, 2014

Contributor

(。´∀`)ノ
Thanks @mosesn @bmdhacks!

Contributor

folone commented Aug 23, 2014

(。´∀`)ノ
Thanks @mosesn @bmdhacks!

cvogt added a commit to cvogt/cbt that referenced this pull request Nov 7, 2016

[split] util-* Convert all tests in util to scalatest
Problem

specs is deprecated and throws a fair number of NullPointerExceptions in scala 2.10
Also all of our tests are disabled in scala 2.10 for sbt

Solution

Port all our tests to scalatest
This is based off of this PR: twitter/util#97

Result

Tests pass

RB_ID=397681

smarter pushed a commit to smarter/twitter-util that referenced this pull request Aug 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment