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

[sinttest] Fix IBR usage (and fix typos) #566

Closed
wants to merge 2 commits into from

Conversation

guusdk
Copy link
Member

@guusdk guusdk commented Sep 29, 2023

Prior to this change, the Smack Integration Test are configured to use In-Band Registration when no admin credentials are provided. However, when trying to apply In-Band Registration, the (missing) admin credentials are used, leading to an error.

In this PR, usage of the admin credentials is prevented when IBR is in use.

Additionally, some typos in a log message and two exception messages are fixed in a separate commit.

The Smack Integration Tests use In-Band Registration when no admin account credentials
are provided. As a consequence, SINT should not try to use those credentials when using
IBR.
@guusdk guusdk changed the title SINT: Fix IBR usage (and fix typos) [sinttest] Fix IBR usage (and fix typos) Sep 29, 2023
@@ -184,20 +184,18 @@ public static boolean removeConnectionDescriptor(String nickname) {

switch (sinttestConfiguration.accountRegistration) {
case serviceAdministration:
case inBandRegistration:
accountRegistrationConnection = defaultConnectionDescriptor.construct(sinttestConfiguration);
accountRegistrationConnection.connect();
accountRegistrationConnection.login(sinttestConfiguration.adminAccountUsername,
Copy link
Member

Choose a reason for hiding this comment

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

Couldn't we simply move this line into the else block below?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe? Having an if/else block in a switch/case that uses the same arguments smells. The fact that I have to think if it would be a suitable alternative makes me prefer my original solution: The code is a lot easier to understand, and isn't even any longer. That goes a long way in preventing the kind of bugs that this PR is fixing.

Copy link
Member

Choose a reason for hiding this comment

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

This approach causes 8 insertions and 9 deletions (if you don't count the deletion of the empty line 194). The reduction in deletions comes due the removal of the if/else block, which accounts for 3 deleted lines.

However, it introduces some duplicate code. And the change itself is much more verbose as compared to #573.

I prefer to avoid duplicate code and hence tend to to with #573 over this.

But in any case, thanks for the bug report!

@Flowdalic
Copy link
Member

Closing in favor of #573

@Flowdalic Flowdalic closed this Nov 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants