Replace specs2 with scalatest #214

mdedetrich · 2023-08-17T14:27:01Z

Resolves: #135
Resolves: #207

This PR replaces specs2 with scalatest which provides the following benefits

Pekko has integrations with scalatest which allows a principled way of setting up tests that uses actors as well as properly cleaning up Actorsystem/pekko-http resources
ScalaTest is much better maintained than specs2 and supports Scala 3
ScalaTest has a concept of fixtures, while its currently use to pass the test name when doing pp it can be extended to do things like auto generate a flowId (even further reducing the boilerplate of current tests)
Can properly test against futures without having to do blocking calls (this is why assertions in scalatest on futures can be placed inside of .map)
Has integrations with testcontainers-scala which can be used to autostart our Nakadi docker container whenever a test, even via Intellij's test runner

I have done some minor improvements but in general I have preferenced minimizing the diff rather than doing more idiomatic improvements (which can be done in future PR's)

mdedetrich · 2023-08-17T14:29:38Z

src/test/scala/org/zalando/kanadi/PekkoTestKitBase.scala

+    Await.result(future, 10 seconds)
+  }
+
+  def pp(obj: Any)(implicit testData: TestData = null) = {


The = null trick can be used to detect whether an implicit TestData is in scope or not.

mdedetrich · 2023-08-17T14:47:26Z

src/test/scala/org/zalando/kanadi/OAuthFailedSpec.scala

-  def oAuthCallSubscriptions = Skipped("No way for current Nakadi docker image to detect \"wrong\" tokens")
-
-  def oAuthPublishEvents = Skipped("No way for current Nakadi docker image to detect \"wrong\" tokens")
+  ignore("No way for current Nakadi docker image to detect \"wrong\" tokens") { _ =>


Scalatest's ignore properly supports having code contained inside which doesn't run unlike with specs2 so I have gone ahead and undid the removal of the test code which happened in the past.

Even though the tests don't work with the current Nakadi docker image, the code illustrates what the expected behaviour is which is useful (and as a bonus if hypothetical future Nakadi docker image ends up supporting this kind of test its trivial to remove the ignore).

mdedetrich · 2023-08-17T15:21:49Z

src/test/scala/org/zalando/kanadi/SubscriptionsSpec.scala

    Await.result(createEventType, 10 seconds)
-
-  override def afterAll = {


I ended up deleting this method because it wasn't actually being run correctly and when I fixed it I got

[info] org.zalando.kanadi.SubscriptionsSpec *** ABORTED *** [info] org.zalando.kanadi.models.GeneralError: Error from server, response is Problem(None,Conflict,409,Some(Can't remove event type Kanadi-Test-Event-be9f9995-7b57-494f-a09f-64355b16d73e, as it has subscriptions),None)

I tried fixing it but I couldn't do so (likely need to put a manual wait) but in any case this cleanup is an artifact of when Kanadi used to test against the actual Zalando test realm (where you did need to clean up subscriptions), and since we run against a docker image this is pointless now so its just better to delete it

mdedetrich · 2023-08-17T15:22:33Z

@gchudnov PR is ready to review

mdedetrich · 2023-08-17T15:25:49Z

src/test/scala/org/zalando/kanadi/SubscriptionsSpec.scala


  def createEventType = eventsTypesClient.create(EventType(eventTypeName, OwningApplication, Category.Business))

-  override def beforeAll =
+  override def beforeAll(): Unit = {
+    super.beforeAll()


You should always call super.beforeAll in case super traits also do some initialisation which is required

mdedetrich · 2023-08-17T15:26:46Z

src/test/scala/org/zalando/kanadi/SubscriptionsSpec.scala

-                                subscriptionsClient.createIfDoesntExist(subscription)
-                              })
-    } yield retrievedSubscription).flatMap(a => a)
+      retrievedSubscription <- Future.sequence(subscriptions.map { subscription =>


Not sure why left arrow wasn't being used here, Future.sequence also returns a future so we should yield on it like another other Future.

coveralls · 2023-08-17T15:27:51Z

Pull Request Test Coverage Report for Build 5906887934

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.1%) to 74.472%

Totals
Change from base Build 5889587227:	0.1%
Covered Lines:	423
Relevant Lines:	568

💛 - Coveralls

mdedetrich · 2023-08-17T15:52:22Z

Converted to draft again because although the tests passed I noticed this

[info] - Retry forever and eventually fail
Error:  [08/17/2023 15:26:00.954] [EventPublishRetrySpec-pekko.actor.internal-dispatcher-2] [pekko://EventPublishRetrySpec/system/IO-TCP/selectors/$a/1] Bind failed for TCP channel on endpoint [localhost/127.0.0.1:34817]
java.net.BindException: [localhost/127.0.0.1:34817] Address already in use
	at java.base/sun.nio.ch.Net.bind0(Native Method)
	at java.base/sun.nio.ch.Net.bind(Net.java:459)
	at java.base/sun.nio.ch.Net.bind(Net.java:448)
	at java.base/sun.nio.ch.ServerSocketChannelImpl.bind(ServerSocketChannelImpl.java:227)
	at java.base/sun.nio.ch.ServerSocketAdaptor.bind(ServerSocketAdaptor.java:80)
	at org.apache.pekko.io.TcpListener.liftedTree1$1(TcpListener.scala:70)
	at org.apache.pekko.io.TcpListener.<init>(TcpListener.scala:67)

Will need to look in more detail

gchudnov

overall looks good thank you!

wondering if that issue with ports is related to EventPublishRetrySpec.scala, where several servers are started (with the same port (?))

mdedetrich · 2023-08-18T08:24:05Z

@gchudnov Don't merge this yet, it seems the tests are always passing due to a qwerk with how async testing is working, currently fixing this now (and it should also helpfully fix the port issue).

mdedetrich · 2023-08-18T10:21:34Z

src/test/scala/org/zalando/kanadi/SubscriptionsSpec.scala

-  eventTypeName.pp
-  s"Consumer Group: $consumerGroup".pp
-
-  def createEventType = eventsTypesClient.create(EventType(eventTypeName, OwningApplication, Category.Business))


This was causing issues when run in beforeAll (likely due to scalatest not expecting to do async calls like this before the tests are run) so I just moved the logic into the main test and I also cleaned up the test so its easier to read

aha, got it

gchudnov · 2023-08-18T16:43:04Z

Don't merge this yet...

yes, and it is not possible to merge a draft PR.
btw, if any help is needed here, please let me know

mdedetrich · 2023-08-18T16:45:57Z

yes, and it is not possible to merge a draft PR. btw, if any help is needed here, please let me know

So the good news is I fixed the fundamental issues with the PR which means the tests are properly running, i.e. I am now using AsyncFlatSpec which means the tests are properly running (unfortunately there isn't an AsyncPropSpec but I think a FlatSpec is cleaner anyways because none of the tests where property based tests and so using PropSpec would be misleading0.

The issues I am dealing with now is that although some of the test suites run in isolation, if you run all of the tests at once there is a race conditions where the tests don't complete. Its probably due to a test/class lifecycle, cleanup with afterAll or something along these lines.

Feel free to do a bit of digging on your side, I am currently playing around with disabling/disabling the http.shutdownAllConnectionPools/Testkit.shutdownActorSystem amongst other things to diagnose whats going on.

gchudnov · 2023-08-18T18:10:16Z

wondering if it helps to serialize the tests on sbt level?

mdedetrich · 2023-08-18T18:44:42Z

wondering if it helps to serialize the tests on sbt level?

I think that theoretically they should already be serialized so its probably something else

Signed-off-by: Matthew de Detrich <mdedetrich@gmail.com>

mdedetrich · 2023-08-18T20:46:44Z

@gchudnov I finally figured out the issue, it was kind of a face palm moment.

Currently with specs2, the order in which the tests are executed is not determined by the test functions (i.e. def createEventType = (name: String) => ... but rather the the test definition at the top of the file i.e.

  override def is: SpecStructure = sequential ^ s2"""
    Create Event Type          $createEventType
    Create Subscription events $createSubscription
    Start streaming            $startStreaming
    Publish events             $publishEvents
    Receive events from source $receiveEvents
    Close connection           $closeConnection
    Delete subscription        $deleteSubscription
    Delete event type          $deleteEventType
    """

It turns out that for the BasicSourceSpec, the order of the SpecStructure was different to the order of the functions defined in the test and I managed to miss this. More specifically the pubish events step was written before start streaming which meant that the test never completed. This is now fixed and the tests are passing.

So to me the PR looks good now, I would also check through it to see if there is anything I may have missed.

gchudnov

Thank you,
merging..

mdedetrich requested review from itachi3, pascalh and gchudnov as code owners August 17, 2023 14:27

mdedetrich commented Aug 17, 2023

View reviewed changes

mdedetrich force-pushed the replace-specs2-with-scalatest branch from 4ca9fab to 7f97a25 Compare August 17, 2023 14:32

mdedetrich marked this pull request as draft August 17, 2023 14:35

mdedetrich force-pushed the replace-specs2-with-scalatest branch 5 times, most recently from f68af08 to fbb35b7 Compare August 17, 2023 14:46

mdedetrich commented Aug 17, 2023

View reviewed changes

mdedetrich force-pushed the replace-specs2-with-scalatest branch 3 times, most recently from 421b260 to adf3a49 Compare August 17, 2023 15:12

mdedetrich commented Aug 17, 2023

View reviewed changes

mdedetrich marked this pull request as ready for review August 17, 2023 15:22

mdedetrich force-pushed the replace-specs2-with-scalatest branch from adf3a49 to 8486f67 Compare August 17, 2023 15:23

mdedetrich commented Aug 17, 2023

View reviewed changes

mdedetrich marked this pull request as draft August 17, 2023 15:51

gchudnov approved these changes Aug 18, 2023

View reviewed changes

mdedetrich force-pushed the replace-specs2-with-scalatest branch 2 times, most recently from 036d613 to cc07a44 Compare August 18, 2023 10:15

mdedetrich commented Aug 18, 2023

View reviewed changes

mdedetrich force-pushed the replace-specs2-with-scalatest branch from cc07a44 to a9e85d0 Compare August 18, 2023 10:43

mdedetrich force-pushed the replace-specs2-with-scalatest branch 2 times, most recently from 969a3a6 to 67f846e Compare August 18, 2023 10:59

Replace specs2 with scalatest

9e8b65d

Signed-off-by: Matthew de Detrich <mdedetrich@gmail.com>

mdedetrich force-pushed the replace-specs2-with-scalatest branch from 67f846e to 9e8b65d Compare August 18, 2023 20:36

mdedetrich marked this pull request as ready for review August 18, 2023 20:46

gchudnov approved these changes Aug 19, 2023

View reviewed changes

gchudnov merged commit f6cc105 into master Aug 19, 2023
7 checks passed

gchudnov deleted the replace-specs2-with-scalatest branch August 19, 2023 13:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace specs2 with scalatest #214

Replace specs2 with scalatest #214

mdedetrich commented Aug 17, 2023 •

edited

Loading

mdedetrich Aug 17, 2023

mdedetrich Aug 17, 2023

mdedetrich Aug 17, 2023 •

edited

Loading

mdedetrich commented Aug 17, 2023

mdedetrich Aug 17, 2023

mdedetrich Aug 17, 2023

coveralls commented Aug 17, 2023 •

edited by github-actions bot

Loading

mdedetrich commented Aug 17, 2023

gchudnov left a comment

mdedetrich commented Aug 18, 2023

mdedetrich Aug 18, 2023

gchudnov Aug 18, 2023

gchudnov commented Aug 18, 2023

mdedetrich commented Aug 18, 2023 •

edited

Loading

gchudnov commented Aug 18, 2023

mdedetrich commented Aug 18, 2023

mdedetrich commented Aug 18, 2023

gchudnov left a comment

		Await.result(createEventType, 10 seconds)

		override def afterAll = {

Replace specs2 with scalatest #214

Replace specs2 with scalatest #214

Conversation

mdedetrich commented Aug 17, 2023 • edited Loading

mdedetrich Aug 17, 2023

Choose a reason for hiding this comment

mdedetrich Aug 17, 2023

Choose a reason for hiding this comment

mdedetrich Aug 17, 2023 • edited Loading

Choose a reason for hiding this comment

mdedetrich commented Aug 17, 2023

mdedetrich Aug 17, 2023

Choose a reason for hiding this comment

mdedetrich Aug 17, 2023

Choose a reason for hiding this comment

coveralls commented Aug 17, 2023 • edited by github-actions bot Loading

Pull Request Test Coverage Report for Build 5906887934

💛 - Coveralls

mdedetrich commented Aug 17, 2023

gchudnov left a comment

Choose a reason for hiding this comment

mdedetrich commented Aug 18, 2023

mdedetrich Aug 18, 2023

Choose a reason for hiding this comment

gchudnov Aug 18, 2023

Choose a reason for hiding this comment

gchudnov commented Aug 18, 2023

mdedetrich commented Aug 18, 2023 • edited Loading

gchudnov commented Aug 18, 2023

mdedetrich commented Aug 18, 2023

mdedetrich commented Aug 18, 2023

gchudnov left a comment

Choose a reason for hiding this comment

mdedetrich commented Aug 17, 2023 •

edited

Loading

mdedetrich Aug 17, 2023 •

edited

Loading

coveralls commented Aug 17, 2023 •

edited by github-actions bot

Loading

mdedetrich commented Aug 18, 2023 •

edited

Loading