-
Notifications
You must be signed in to change notification settings - Fork 10
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
Integration test with multiple aca-py agents #13
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Ian Costanzo <[email protected]>
Signed-off-by: Ian Costanzo <[email protected]>
Signed-off-by: Ian Costanzo <[email protected]>
Signed-off-by: Ian Costanzo <[email protected]>
Signed-off-by: Ian Costanzo <[email protected]>
Signed-off-by: Ian Costanzo <[email protected]>
assertTrue(conn2_updated.isPresent()); | ||
|
||
// pause to allow the agent chatter to complete | ||
Thread.sleep(4000); |
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.
Thread.sleep() is something that is never good in any test. As it slows down each test execution and is not portable between machines. If there is really no other way it is better to have a waiting condition with a callback that checks for the expected change and fails if the change does not happen in a certain amount of time.
import static org.junit.jupiter.api.Assertions.*; | ||
|
||
@Slf4j | ||
public class MultiConnectionRecordTest extends MultiIntegrationTestBase { |
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.
As these tests are integration tests they should not run together with the other tests. To achieve that I would recommend using the @Tag("IntegrationTest")
annotation together with a filter in the surefire plugin:
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-surefire-plugin</artifactId>
<version>3.0.0-M5</version>
<configuration>
<excludedGroups>IntegrationTest</excludedGroups>
</configuration>
</plugin>
To run the integration tests only one can either add a profile to the pom or do this via the command line e.g. mvn test -Dgroups=IntegrationTest
@Slf4j | ||
public class EndorserConnectionRecordTest extends MultiIntegrationTestBase { | ||
|
||
public static final String INDY_LEDGER_URL = "https://indy-test.bosch-digital.de"; |
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.
How complicated is it to start a indy node via a test container? This would probably be the best solution instead of relying to an external service that we wanted to turn off a while ago.
.withLogConsumer(new Slf4jLogConsumer(log)) | ||
; | ||
|
||
protected String extraAgentArgs(int agentNum) { |
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 api is a bit confusing, I would also make that method abstract.
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.
And I would split this up into two abstract methods like agentOneArgs() and agentTwoArgs, then you do not have to use switches all the time
.post(jsonBody(gson.toJson(body))) | ||
.build(); | ||
} | ||
|
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.
Dead code below
.alias(alias_1) | ||
.did(localDid1.getDid()) | ||
.verkey(localDid1.getVerkey()) | ||
.build()); |
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.
I like using orElseThrow() in these cases, makes it cleaner to read.
@lombok.NoArgsConstructor | ||
@lombok.Builder | ||
public class LedgerDIDCreate { | ||
public static final String SERIALIZED_NAME_ALIAS = "alias"; |
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.
using @SerialisedName
here is redundant
@lombok.AllArgsConstructor | ||
@lombok.NoArgsConstructor | ||
@lombok.Builder | ||
public class LedgerDIDCreate { |
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.
Make these two inner classes of the LedgerClient?
*/ | ||
@Slf4j | ||
@SuppressWarnings("unused") | ||
public class LedgerClient extends BaseClient { |
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.
Would be useful to have this in the main package
@lombok.Builder | ||
public class LedgerDIDResponse { | ||
public static final String SERIALIZED_NAME_DID = "did"; | ||
@SerializedName(SERIALIZED_NAME_DID) |
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.
Same as above
Signed-off-by: Ian Costanzo <[email protected]>
@etschelp the build for this PR is failing with a license check, however it look like I've included the license header in each of these files, any suggestions?
|
@ianco simply run |
Signed-off-by: Ian Costanzo <[email protected]>
Awesome thanks for the tip! |
Test connection process between 2 agents.
New base class for integration testing with multiple agents.
Parametrize agent startup to allow different tests to start agents with different settings.
Add test with author/endorser flow.