[sinttest] Additional tests covering s7 of XEP-0045

[Fishbowler]

Follows #475, #477 and #478. Draft until they're merged.

[evdherberg]

This morning I rebased this branch to Fishbowler:xep-0045-coverage-part3

[evdherberg]

@Flowdalic My rebase may have cleared out your earlier comments, which is unfortunate. I remember one about using enums, probably for error types. Do you remember the other one?

[Flowdalic]

The are still at the commit where I made them (which was "dropped" by you force pushing): 65dee17

[Flowdalic]

Not directly related to this PR, but the following pattern repeats over and over in the MUC integration tests

        EntityBareJid mucAddress = getRandomRoom("smack-inttest-discoinfo");
        MultiUserChat mucAsSeenByOne = mucManagerOne.getMultiUserChat(mucAddress);
        createMuc(mucAsSeenByOne, Resourcepart.from("one-" + randomString));

which suggests that we can add a convenience method to reduce the boilerplate code. I thnk of

public MultiUserChat createMuc(MultiUserChatManager manager, String roomnameComponent) {
…
}

which, for example, determines the nickname prefix (one-, two-) by the provided manager.

Note that I was not able to review the complete PR yet. But I'd like to provide my feedback early, even if it is incomplete.

[Flowdalic]

Did you check that no concurrent access to this data structure happens?

[Fishbowler]

For the ArrayList?

[Flowdalic]

Yes

[Fishbowler]

Not directly related to this PR, but the following pattern repeats over and over in the MUC integration tests

        EntityBareJid mucAddress = getRandomRoom("smack-inttest-discoinfo");
        MultiUserChat mucAsSeenByOne = mucManagerOne.getMultiUserChat(mucAddress);
        createMuc(mucAsSeenByOne, Resourcepart.from("one-" + randomString));

which suggests that we can add a convenience method to reduce the boilerplate code. I thnk of

public MultiUserChat createMuc(MultiUserChatManager manager, String roomnameComponent) {
…
}

which, for example, determines the nickname prefix (one-, two-) by the provided manager.

I've had a play with this, but we regularly need mucAsSeenByOne later in the test. You could wrap the first two lines and return the MultiUserChat, but you couldn't use it if you needed a mucAsSeenByTwo, or you'd need to get the JID from mucAsSeenByOne, and we'd save no lines and reduce complexity even less.

[Flowdalic]

This duplicated code can be removed by slightly changing the logic of this method.

[guusdk]

What duplication are you referring to here? I don't see how we can reduce the check if the expected count matches the result count, as that needs to happen both before and after waiting. (This mimics what's implemented in ResultSyncPoint).

[Flowdalic]

First, I just noticed that this method directly returns results. I am not sure if this is sensible, given that results could potentially concurrently be modified after it was returned. Therefore I would suggest returning a copy, where the copy is made while the monitor lock is held.

I suggest the following, IMHO more idiomatic, implementation for the method. Also note that it follows a more defensive approach, where the exception is thrown, even if all results are there. If there is a compelling reason to swap this, then I could consider that. (It would also be made configurable)

    public synchronized List<R> waitForResults(long timeout) throws E, InterruptedException, TimeoutException {
        long now = System.currentTimeMillis();
        final long deadline = now + timeout;
        while (results.size <= expectedResultSize && !exception && now < deadline) {
              wait(deadline - now);
              now = System.currentTimeMillis();
        }
         if (now >= deadline) throw new TimeoutException("Timeout waiting " + timeout + " millis");
         if (exception != null) throw exception;
         return results; // TODO: Copy results
    }

[Flowdalic]

I've had a play with this, but we regularly need mucAsSeenByOne later in the test. You could wrap the first two lines and return the MultiUserChat, but you couldn't use it if you needed a mucAsSeenByTwo, or you'd need to get the JID from mucAsSeenByOne, and we'd save no lines and reduce complexity even less.

How about?

MultiUserChat mucAsSeenByOne = createMuc(mucManagerOne, "smack-inttest-discoinfo");
MultiUserChat musAsSeenByTwo = getMuc(mucManagerTwo, mucAsSeenByOne);

[guusdk]

Not directly related to this PR, but the following pattern repeats over and over in the MUC integration tests

        EntityBareJid mucAddress = getRandomRoom("smack-inttest-discoinfo");
        MultiUserChat mucAsSeenByOne = mucManagerOne.getMultiUserChat(mucAddress);
        createMuc(mucAsSeenByOne, Resourcepart.from("one-" + randomString));

which suggests that we can add a convenience method to reduce the boilerplate code. I thnk of

public MultiUserChat createMuc(MultiUserChatManager manager, String roomnameComponent) {
…
}

which, for example, determines the nickname prefix (one-, two-) by the provided manager.

I've tried doing this, but it doesn't really work out most of the time. Many of the instance references that are removed by such a method are needed elsewhere in the unit test implementation (for example in the assertions) making the replacement effort rather moot.

[guusdk]

@Flowdalic - is there anything to be done to 'fix' the coveralls check (that's now reporting failure)?

[guusdk]

@Flowdalic - is there anything to be done to 'fix' the coveralls check (that's now reporting failure)?

I've now added a new unit test for the MultiResultSyncPoint util that was added in this PR. Hopefully, that makes the test coverage check happy again.

[Flowdalic]

First, I just noticed that this method directly returns results. I am not sure if this is sensible, given that results could potentially concurrently be modified after it was returned. Therefore I would suggest returning a copy, where the copy is made while the monitor lock is held.

I suggest the following, IMHO more idiomatic, implementation for the method. Also note that it follows a more defensive approach, where the exception is thrown, even if all results are there. If there is a compelling reason to swap this, then I could consider that. (It would also be made configurable)

    public synchronized List<R> waitForResults(long timeout) throws E, InterruptedException, TimeoutException {
        long now = System.currentTimeMillis();
        final long deadline = now + timeout;
        while (results.size <= expectedResultSize && !exception && now < deadline) {
              wait(deadline - now);
              now = System.currentTimeMillis();
        }
         if (now >= deadline) throw new TimeoutException("Timeout waiting " + timeout + " millis");
         if (exception != null) throw exception;
         return results; // TODO: Copy results
    }

[guusdk]

Applied review feedback, and squashed ~3 years of commits into one.

[Flowdalic]

One minor thing, not sure why did only noticed it yet.