-
Notifications
You must be signed in to change notification settings - Fork 5
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
chore: removing it profile #63
Conversation
ci, parent pom and moving samples 'it' to 'test'
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.
This is looking really good. Much nicer!
I left a few comments related to what we discussed today.
.github/workflows/ci.yml
Outdated
@@ -440,7 +440,7 @@ jobs: | |||
fi | |||
if [ true == '${{matrix.it}}' ]; then | |||
${PRE_CMD} | |||
KALIX_TESTKIT_DEBUG=true mvn verify -Pit --no-transfer-progress | |||
KALIX_TESTKIT_DEBUG=true mvn verify --no-transfer-progress |
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.
we can change this to AKKA_TESTKIT_DEBUG, but later.
@@ -11,7 +11,7 @@ | |||
* It interacts with the components of the application using a componentClient | |||
* (already configured and provided automatically through injection). | |||
*/ | |||
public class IntegrationTest extends TestKitSupport { | |||
public class HelloWorldIT extends TestKitSupport { |
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.
We can keep this as IntegrationTest
because IntegrationTest
is excluded from the mvn test
. Also, HelloWorldIT
is completely unrelated with the archetype.
IMO, we should prefer suffixing the test with IntegrationTest
instead of IT
in all cases because it makes it more obvious.
I understand that the folks from the fail failsafe plugin had to choose for IT
, because they don't know where it will be used. So they needed to step out of *IntegrationTest
to avoid conflicts with the more general *Test
.
But our parent pom configures the failsafe plugin and the surefire plugin. So we don't need to favor IT
.
Then we have:
*Test
for unit tests, picked by surefire.*IntegratrionTest
are excluded.*IntegratrionTest
and all the variants withIT
(as originally define by failsafe plugin). Those will be picked by failsafe plugin.
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.
Ok, my idea was to follow the convention but I think you make a valid point. It's for sure clearer.
@@ -51,7 +51,7 @@ | |||
import static org.assertj.core.api.Assertions.assertThat; | |||
|
|||
@ExtendWith(Junit5LogCapturing.class) | |||
public class SdkIntegrationTest extends TestKitSupport { |
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.
Let's revert them all back to *IntegrationTest
as per other comment.
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.
Alright.
Co-authored-by: Renato Cavalcanti <[email protected]>
Co-authored-by: Renato Cavalcanti <[email protected]>
Co-authored-by: Renato Cavalcanti <[email protected]>
Co-authored-by: Renato Cavalcanti <[email protected]>
Co-authored-by: Renato Cavalcanti <[email protected]>
.+ back to IntegrationTest suffix
This reverts commit 493648d.
This reverts commit 29311bf.
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.
This is really nice DX improvement!
Bringing https://github.com/lightbend/akka-javasdk/pull/652
Changes