Migrate finagle-thrift/finagle-stream tests to ScalaTest #291

bajohns · 2014-07-05T07:17:13Z

In preparation for 2.11.x finagle-thrift tests will migrate to ScalaTest

Remaining Work:

Finagle-Streams
2.11.x testing
Conversation about FinagleClientThriftServerSpec structure
Renaming tests from *Spec -> *Test
Clean up commit history and conform to commit message standards

bajohns · 2014-07-05T07:23:48Z

@mosesn - In issue #290 you mentioned testing 2.11.x compatibility but I have not seen a published 2.11 version of twitter/util.

Should I use falone/util - and publish-local - to test 2.11?

mosesn · 2014-07-05T14:58:06Z

Yeah, that sounds promising. We'll probably publish our first 2.11 util in the next couple weeks.

Problem ThriftTest has testing functionality that was marked flaky but currently passes. I am unsure if these are desired tests but they pass and may be useful. Solution Given that there is a convention for including flaky tests by default - and disabling via ENV variable - I reenabled test paths that pass. For paths that do not pass I created a skipThriftTest method. Result Potentially higher test coverage in the package.

bajohns · 2014-07-13T04:39:36Z

@mosesn - I completed the specs -> scalatest conversion for finagle-thrift and finagle-stream.

Please let me know any feedback you may have so I can rebase the work into a set of final commits that conform to the commit standard.

Lastly, I came across two issues that I would like some more input on:

finagle-thrift has test functionality in ThriftTest.scala that seems useful and is marked flaky. I uncommented parts of it and migrated it to checking an environment variable to determine whether to run or not. Based on other tests, this looks like the new convention for dealing with tests that have intermittent failures. Please see this commit: bajohns@1531023
I have not found a good solution to replace the specs2 matcher must beLike. It is used a number of times in finagle-thrift and in a nested fashion in finagle-stream specs scalatest . Please let me know if you have other methods for migrating that construct.

mosesn · 2014-07-16T15:33:11Z

Hey @bajohns this looks great! Unfortunately, I've been snowed in work-wise, and haven't had a chance to take a look at this yet. It's still on my plate, but it will take a little while to get to it. Sorry! 😬

bajohns · 2014-07-26T22:32:05Z

Hey @mosesn, no rush I see there is a lot of activity around these change sets. Thanks for all your work to accept contributions!

evnm · 2014-07-30T21:39:54Z

finagle-thrift/src/test/scala/com/twitter/finagle/thrift/AsyncServerEndToEndTest.scala

+  val protocolFactory = new TBinaryProtocol.Factory()
+
+  test("async Thrift server should work"){
+    // ** Set up the server.


Rm the ** while we're in here.

bajohns · 2014-08-04T02:07:01Z

@evnm - Thanks for reviewing. All comments handling in next commit: bajohns@255983d

bajohns · 2014-08-09T17:07:40Z

Looks like the build failed only for artifact resolution. Rebuild should clear it out.

[error] (finagle-commons-stats/*:update) sbt.ResolveException: download failed: com.google.inject#guice;3.0!guice.jar
[error] Total time: 11 s, completed Aug 4, 2014 2:04:13 AM

mosesn · 2014-08-10T10:14:34Z

finagle-stream/src/test/scala/com/twitter/finagle/stream/EndToEndTest.scala

+import org.jboss.netty.handler.codec.http._
+import org.scalatest.{OneInstancePerTest, FunSuite}
+
+class EndToEndTest extends FunSuite with OneInstancePerTest {


I don't love the style of OneInstancePerTest, it's not particularly idiomatic scalatest. Generally we just make a context class and import its contents in each test. By sticking to one style, it's easier for us to reason about different tests, and by having tightly scoped context classes, we can have more sophisticated fixtures if we have a big test suite, for example.

mosesn · 2014-08-10T10:23:05Z

finagle-thrift/src/test/scala/com/twitter/finagle/thrift/ClientIdContextTest.scala

-  with BeforeAndAfter
-  with OneInstancePerTest
-{
+class ClientIdContextTest extends FunSuite with BeforeAndAfter with OneInstancePerTest {


I don't like this style, but probably not worth changing, since you don't need to change it for any other reason.

mosesn · 2014-08-10T10:39:00Z

LGTM other than a couple style nits. The context class vs. OneInstancePerTest thing is up to you--I prefer having a single paradigm across finagle, and OneInstancePerTest is only used twelve times in finagle, but it does make the code cleaner, so maybe we'll want to investigate just using OneInstancePerTest across the board in the future.

bajohns · 2014-08-15T21:19:24Z

I am going to take care of the feedback this weekend. I am more than willing to convert the tests from OneInstancePerTest to the context object - this would make the changeset larger. The specs default behavior is OneInstancePerTest-like so the shortest path forward was OIPT.

Unless I hear otherwise - I will do the work to migrate this from OIPT -> Context object pattern.

bajohns · 2014-09-01T03:56:42Z

@mosesn - finished reviewing your feedback. Please confirm that my changes meet your expectations.

Re: ignore showing up for flaky - here is the test output https://gist.github.com/bajohns/e1a03cad4b401ae5fd83

mosesn · 2014-09-03T04:48:13Z

This is looking pretty awesome, thanks a ton! 👍

LGTM

bajohns · 2014-09-03T04:54:31Z

Great to hear - would you like me to squash commits down to a single? That was my plan to conform to the commit message standard.

mosesn · 2014-09-03T06:31:15Z

That would be rad, it would probably help minimize conflicts too. Could you make a new PR though, so the history for this branch is still one to reason about?

evnm · 2014-09-03T19:57:44Z

finagle-stream/src/test/scala/com/twitter/finagle/stream/EndToEndTest.scala

+
+class EndToEndTest extends FunSuite {
+
+  case class MyStreamResponse(


This indentation's a bit weird. Let's use:

case class MyStreamResponse( httpResponse: HttpResponse, messages: Offer[ChannelBuffer], error: Offer[Throwable] ) extends StreamResponse {

evnm · 2014-09-03T20:02:54Z

LGTM. Just some dumb style nits.

bajohns · 2014-09-07T04:25:05Z

Took care of feedback from @evnm

I will create a final pull request based on these commits.

bajohns · 2014-09-07T05:16:16Z

Squashed and rebased against current master - the complete work in one commit is here: #308

maciejjaskowski · 2014-09-25T07:20:04Z

Any chances for this PR being merged ?

mosesn · 2014-09-25T13:10:50Z

We're in the process of doing it today! 🎩

bajohns · 2014-10-04T04:29:13Z

Looks like the other was used for internal merge. Closing this.

Thanks again @mosesn and @evnm

bajohns added 5 commits July 5, 2014 01:47

First round of thrift tests on scala test

dfa9269

Second round of tests

b62c5a4

Finagle ClientServer test to scala test

a6d8f1a

Formatting changes

69b49c6

Convert ValidateThriftServiceSpec

5d6d2ad

bajohns added 2 commits July 12, 2014 14:08

Rename all Spec -> Test and format

955e0e7

Convert single finagle-stream test to scala-test style

5e08ca9

bajohns changed the title ~~WIP Migrate finagle-thrift tests to ScalaTest~~ WIP Migrate finagle-thrift/finagle-stream tests to ScalaTest Jul 13, 2014

bajohns added 2 commits July 12, 2014 23:39

Formatting and style fixes

11e9081

bajohns changed the title ~~WIP Migrate finagle-thrift/finagle-stream tests to ScalaTest~~ Migrate finagle-thrift/finagle-stream tests to ScalaTest Jul 13, 2014

evnm reviewed Jul 30, 2014
View reviewed changes

Initial PR feedback (spacing and indents)

255983d

mosesn reviewed Aug 10, 2014
View reviewed changes

Feedback for Context class usage over OIPT

ce93902

evnm reviewed Sep 3, 2014
View reviewed changes

Handle PR formating feedback

16f2a50

bajohns force-pushed the thrift-to-scala-test branch from b524ea8 to 16f2a50 Compare September 7, 2014 04:23

bajohns mentioned this pull request Sep 7, 2014

Migration of tests in finagle-thrift and finagle-stream to ScalaTest #308

Closed

bajohns closed this Oct 4, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate finagle-thrift/finagle-stream tests to ScalaTest #291

Migrate finagle-thrift/finagle-stream tests to ScalaTest #291

bajohns commented Jul 5, 2014

bajohns commented Jul 5, 2014

mosesn commented Jul 5, 2014

bajohns commented Jul 13, 2014

mosesn commented Jul 16, 2014

bajohns commented Jul 26, 2014

evnm Jul 30, 2014

bajohns commented Aug 4, 2014

bajohns commented Aug 9, 2014

mosesn Aug 10, 2014

mosesn Aug 10, 2014

mosesn commented Aug 10, 2014

bajohns commented Aug 15, 2014

bajohns commented Sep 1, 2014

mosesn commented Sep 3, 2014

bajohns commented Sep 3, 2014

mosesn commented Sep 3, 2014

evnm Sep 3, 2014

evnm commented Sep 3, 2014

bajohns commented Sep 7, 2014

bajohns commented Sep 7, 2014

maciejjaskowski commented Sep 25, 2014

mosesn commented Sep 25, 2014

bajohns commented Oct 4, 2014


		class EndToEndTest extends FunSuite {

		case class MyStreamResponse(

Migrate finagle-thrift/finagle-stream tests to ScalaTest #291

Migrate finagle-thrift/finagle-stream tests to ScalaTest #291

Conversation

bajohns commented Jul 5, 2014

bajohns commented Jul 5, 2014

mosesn commented Jul 5, 2014

bajohns commented Jul 13, 2014

mosesn commented Jul 16, 2014

bajohns commented Jul 26, 2014

evnm Jul 30, 2014

Choose a reason for hiding this comment

bajohns commented Aug 4, 2014

bajohns commented Aug 9, 2014

mosesn Aug 10, 2014

Choose a reason for hiding this comment

mosesn Aug 10, 2014

Choose a reason for hiding this comment

mosesn commented Aug 10, 2014

bajohns commented Aug 15, 2014

bajohns commented Sep 1, 2014

mosesn commented Sep 3, 2014

bajohns commented Sep 3, 2014

mosesn commented Sep 3, 2014

evnm Sep 3, 2014

Choose a reason for hiding this comment

evnm commented Sep 3, 2014

bajohns commented Sep 7, 2014

bajohns commented Sep 7, 2014

maciejjaskowski commented Sep 25, 2014

mosesn commented Sep 25, 2014

bajohns commented Oct 4, 2014