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

catch the TimeoutException #9

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Original file line number Diff line number Diff line change
Expand Up @@ -947,4 +947,49 @@ public void deleteNonExistentNodeTest() throws NoResponseException, XMPPErrorExc
assertEquals(StanzaError.Condition.item_not_found, e.getStanzaError().getCondition());
}
}

/**
* Assert that the server send a notification to subscribers when deleting a
* node that exist
*
* <p>
* From XEP-0060 § 8.4.2:
* </p>
* <blockquote> In order to delete a node, a node owner MUST send a node
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't the correct quote. It does not describe what you're testing here!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not yet the complete test. It's just a step

Copy link
Contributor

Choose a reason for hiding this comment

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

I know. Just leave this comment open until you fixed it. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

The quote still is not correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have already changed the quote

* deletion request, consisting of a &lt;delete/&gt; element whose 'node'
* attribute specifies the NodeID of the node to be deleted </bloquote>
*
* @throws NoResponseException if there was no response from
* the remote entity.
* @throws XMPPErrorException if there was an XMPP error
* returned.
* @throws NotConnectedException if the XMPP connection is not
* connected.
* @throws InterruptedException if the calling thread was
* interrupted.
* @throws PubSubException.NotAPubSubNodeException if the node cannot be
* accessed.
*/
@SmackIntegrationTest
public void deleteNodeAndNotifySubscribersTest() throws NoResponseException, XMPPErrorException,
NotConnectedException, InterruptedException, TimeoutException, ExecutionException {
final String nodename = "sinttest-delete-node-that-exist-" + testRunId;
final String needle = "<event xmlns='http://jabber.org/protocol/pubsub#event'>";
final String delete_confirm = "<delete node='princely_musings'>";
Copy link
Contributor

Choose a reason for hiding this comment

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

This won't work. You're creating a node wit a name that starts with sinttest-delete-node-that-exists-, but you're deleting a node that is named princely_musings.

final String regex = "#^." + needle + "." + delete_confirm + ".$#";
try {
pubSubManagerOne.createNode(nodename);
final Node subscriberNode = pubSubManagerTwo.getNode(nodename);
final EntityBareJid subscriber = conTwo.getUser().asEntityBareJid();
subscriberNode.subscribe(subscriber);
final CompletableFuture<Stanza> result = new CompletableFuture<>();
conTwo.addAsyncStanzaListener(result::complete, stanza -> stanza.toXML("").toString().matches(regex));
// Delete an existent node
pubSubManagerOne.deleteNode(nodename);
assertNotNull(result.get(conOne.getReplyTimeout(), TimeUnit.MILLISECONDS));
}
catch (PubSubException.NotAPubSubNodeException e) {
throw new AssertionError("The published item was not received by the subscriber.", e);
Copy link
Contributor

Choose a reason for hiding this comment

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

A test can have three results:

  • All assertions pass: test succeeds. This means that the test was executed, and the thing that we are testing is working as expected.
  • One of the assertions does not pass: test fails. This means that the test was executed, and the thing that we are testing is not working as expected.
  • An error occurs: tests in error. This means that the test was not executed properly at all. This might indicate that there is a problem with the test code, or something else unexpected happened.

Assertions are used to verify expected behavior. When you write a test, you test for a very specific thing to happen. That is where you use an assertion: if the things does not happen, the assertion fails. This means that the test fails.

If something unexpected happens - something that went wrong, without it being the subject of the test - then the test should exit in error.

By catching PubSubException.NotAPubSubNodeException and throwing an AssertionError you are causing the test to fail. Is that really what you want here (are you testing a scenario in which a PubSubException.NotAPubSubNodeException can be expected to happen, normally?) I think that instead, the code should simply cause the original exception to be thrown. You don't need to 'catch' it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sometimes I used to have errors due to the duration too long when I made tests

}
}
}