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

gardening: remove use of IntegrationTest scope #1666

Closed
wants to merge 19 commits into from

Conversation

ennru
Copy link
Member

@ennru ennru commented Aug 24, 2023

The IntegrationTest scope is deprecated since sbt 1.9.

Moving integration tests to a separate project instead which now depends on the tests project for shared code and dependencies.

Refs

@ennru
Copy link
Member Author

ennru commented Sep 29, 2023

The integration tests ran and worked, but I don't see why "Build and Test" didn't execute properly.

@ennru
Copy link
Member Author

ennru commented Nov 11, 2023

Rebased on main

Copy link
Member

@johanandren johanandren left a comment

Choose a reason for hiding this comment

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

Looks good but have opinions on test code in src/main vs making the dependency test scoped.

Copy link
Member

@patriknw patriknw left a comment

Choose a reason for hiding this comment

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

I see that I reviewed this a long while ago but never submitted my comments. Here they are. Not sure if they are outdated by now?

build.sbt Outdated Show resolved Hide resolved
.settings(
name := "akka-stream-kafka-tests",
libraryDependencies ++= Seq(
"com.typesafe.akka" %% "akka-discovery" % akkaVersion,
"com.google.protobuf" % "protobuf-java" % "3.23.3", // use the same, or later, version as in scalapb
"org.testcontainers" % "kafka" % testcontainersVersion,
Copy link
Member

Choose a reason for hiding this comment

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

I would have expected that this project would not need test containers, but that would be in integrationTests? Benchmarks could depend on integrationTests if that is the reason.
One advantage of that is that it's more clear when someone adds such tests that it should be in the integrationTests project and not in tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

Most Alpakka Kafka tests need a Kafka instance to talk to, they share a Kafka provided via TestContainers.

The difference for the Integration Tests is that they run test specific Kafkas TestcontainersKafkaPerClassLike with specific configuration.

@ennru
Copy link
Member Author

ennru commented Nov 20, 2023

Looks good but have opinions on test code in src/main vs making the dependency test scoped.

Very few classes from 'tests' are referenced from the integrationTests. The cleanest solution would be to introduce a tiny internal test kit.

Copy link
Member

@patriknw patriknw left a comment

Choose a reason for hiding this comment

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

LGTM

@johanandren
Copy link
Member

One thought after starting on something parallel in akka-management, can we follow a common naming scheme for these, I think the recommendation in the sbt deprecation was integration or aaa-integration for multi-module builds with separate integration tests. Would be tidy to follow that across Akka projects.

@ennru
Copy link
Member Author

ennru commented Feb 21, 2024

Build success!
But tests/test takes much longer than integrationTests/test. I'll look at moving more tests to integrationTests.

@ennru ennru force-pushed the ennru-move-it branch 2 times, most recently from 7f10dd3 to 45dd000 Compare March 8, 2024 11:07
@ennru
Copy link
Member Author

ennru commented Mar 8, 2024

Rebased in the hope that the updated GH actions from main will help mitigating the full device problem.

@ennru
Copy link
Member Author

ennru commented Mar 8, 2024

System.IO.IOException: No space left on device : '/home/runner/runners/2.314.1/_diag/Worker_20240308-111255-utc.log'

@johanandren
Copy link
Member

Something to try: actions/runner-images#2840 (comment)

@ennru ennru closed this Sep 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants