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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 2 commits into from
Closed

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

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
37c024e
[sinttest] When using IBR, do not attempt to login
guusdk Sep 29, 2023
efa71ca
[sinttest] Fix typos in log and exception messages
guusdk Sep 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 8 additions & 10 deletions ...ntegration-test/src/main/java/org/igniterealtime/smack/inttest/XmppConnectionManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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!

sinttestConfiguration.adminAccountPassword);

if (sinttestConfiguration.accountRegistration == AccountRegistration.inBandRegistration) {

adminManager = null;
accountManager = AccountManager.getInstance(accountRegistrationConnection);
} else {
adminManager = ServiceAdministrationManager.getInstanceFor(accountRegistrationConnection);
accountManager = null;
}
adminManager = ServiceAdministrationManager.getInstanceFor(accountRegistrationConnection);
accountManager = null;
break;
case inBandRegistration:
accountRegistrationConnection = defaultConnectionDescriptor.construct(sinttestConfiguration);
accountRegistrationConnection.connect();
adminManager = null;
accountManager = AccountManager.getInstance(accountRegistrationConnection);
break;
case disabled:
accountRegistrationConnection = null;
Expand Down