-
Notifications
You must be signed in to change notification settings - Fork 645
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
Less use of deprecated APIs in Java code #2413
Conversation
tests: use Hamcrest's assertThat JMS tests: expect connection to be closed eventually File tests: Check that the file exists, eventually on Travis Slick test: recover -> recoverWithRetries
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. A few comments.
@@ -5,7 +5,11 @@ | |||
package akka.stream.alpakka.amqp.javadsl; | |||
|
|||
import static org.hamcrest.CoreMatchers.instanceOf; | |||
import static org.junit.Assert.*; | |||
import static org.hamcrest.MatcherAssert.assertThat; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[question] Why hamcrest's assertThat
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The JUnit variant is deprecated nowadays.
@@ -539,7 +539,7 @@ class JmsConnectorsSpec extends JmsSpec { | |||
val completionFuture: Future[Done] = Source(msgsIn).runWith(jmsSink) | |||
completionFuture.futureValue shouldBe Done | |||
// make sure connection was closed | |||
connectionFactory.cachedConnection shouldBe 'closed | |||
eventually { connectionFactory.cachedConnection shouldBe Symbol("closed") } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[minor] Don't like the symbol shorthand?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's deprecated in Scala 2.13.
private static ActorSystem setupSystem() { | ||
final ActorSystem system = ActorSystem.create("MqttFlowTest"); | ||
final Materializer materializer = ActorMaterializer.create(system); | ||
return Pair.create(system, materializer); | ||
return system; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[minor] Don't really need this method anymore.
@@ -194,7 +190,8 @@ lazy val googleCloudPubSubGrpc = alpakkaProject( | |||
"-P:silencer:pathFilters=akka-grpc/main", | |||
"-P:silencer:pathFilters=akka-grpc/test" | |||
), | |||
crossScalaVersions --= Seq(Dependencies.Scala211) // 2.11 is not supported since Akka gRPC 0.6 | |||
compile / javacOptions := (compile / javacOptions).value.filterNot(_ == "-Xlint:deprecation"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[minor] This effectively just sets -Xlint:unchecked
. We could set this setting to that instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, we might want to add more linting in Common.
tests: use Hamcrest's assertThat
JMS tests: expect connection to be closed eventually
File tests: Check that the file exists, eventually on Travis
Slick test: recover -> recoverWithRetries
A part of #2410