-
Notifications
You must be signed in to change notification settings - Fork 9
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
DBZ-7155 Add Unit Tests for ServiceAccountDependent Class #26
Conversation
hi @jcechace, Could you please take a look on this PR to add unittests on the ServiceAccountDependent class. Thank you |
727b91f
to
420eb8a
Compare
} | ||
|
||
@Test | ||
void shouldReturnManagerServiceAccount() { |
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.
@ilyasahsan123 this should probably read shouldReturnManagedServiceAccount
ServiceAccount serviceAccount = serviceAccountDependent.desired(debeziumServer, context); | ||
|
||
assertThat(serviceAccount.getMetadata().getName()).isEqualTo(managedSaName); | ||
assertThat(serviceAccount.getMetadata().getNamespace()).isEqualTo("default"); |
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.
default
is the default so IMHO it might be best to change the namespace first and tests with that.
} | ||
|
||
@Test | ||
void shouldDesiredServiceAccount() { |
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.
Likely should read shouldReturnDesiredServiceAccount
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.
@ilyasahsan123 Also please squash the commits. Thanks!
07016f7
to
c68d2cd
Compare
|
||
@AfterEach | ||
void after() { | ||
client.resource(debeziumServer).delete(); |
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.
@ilyasahsan123 since this is a unit test we are not really creating the object within kubernetes (we shouldn't). So I don't think this is required. Consequently the client field itself is not required.
private final String dsName = "test-ds"; | ||
private final String saName = "test-ds-service-account"; | ||
private final String managedSaName = "test-ds-sa"; | ||
private final String namespace = "debezium"; |
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.
These could be constants, no?
c68d2cd
to
77e5a99
Compare
private final String managedSaName = "test-ds-sa"; | ||
private final String namespace = "debezium"; |
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.
Great. Just convert these two into constants (e.g. private static final String MANAGED_SA_NAME
) and I will merge it.
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.
Done. 👍
add end of lines Reformat the file Fix the failed CI by sorting the imports. Fix the failed CI by adding Class header. Implement Suggestion from peer review refactor the code Remove Kubernetes Client Rename constant variables
77e5a99
to
d380815
Compare
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.
Thanks for contributing!
https://issues.redhat.com/browse/DBZ-7155