Tinder

Application-Specific Error Conditions

Details

  • Type: Improvement Improvement
  • Status: Resolved Resolved
  • Priority: Minor Minor
  • Resolution: Fixed
  • Affects Version/s: None
  • Fix Version/s: 1.1 - Concurrency
  • Component/s: None
  • Labels:
    None
  • Acceptance Test - Add?:
    No

Description

http://www.igniterealtime.org/community/thread/38340

I wrote a patch for the PacketError class to add support for application-specific error conditions. It would be great if it gets into the trunk of Openfire or start a discussion about that topic.

Best Regards
Guenther Niess

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Guus der Kinderen added a comment -

Hey Guenther, I've been looking at that patch. I think one thing could be improved: The behavior of the methods you added is a bit off, if a user attempts to insert an application-specific error, using the namespace urn:ietf:params:xml:ns:xmpp-stanzas. According to http://xmpp.org/rfcs/rfc3920.html#stanzas-error-app, this is illegal. We should implement that restriction.

Additionally, could you please provide junit tests that verify the correct behavior of your methods, both for valid as invalid input params?

Show
Guus der Kinderen added a comment - Hey Guenther, I've been looking at that patch. I think one thing could be improved: The behavior of the methods you added is a bit off, if a user attempts to insert an application-specific error, using the namespace urn:ietf:params:xml:ns:xmpp-stanzas. According to http://xmpp.org/rfcs/rfc3920.html#stanzas-error-app, this is illegal. We should implement that restriction. Additionally, could you please provide junit tests that verify the correct behavior of your methods, both for valid as invalid input params?
Hide
Permalink
Guenther Niess added a comment - - edited

Thanks Guus for looking into my patch and giving me the chance to extend it. I've added a check for the namespace and unit test.

Show
Guenther Niess added a comment - - edited Thanks Guus for looking into my patch and giving me the chance to extend it. I've added a check for the namespace and unit test.
Hide
Permalink
Guus der Kinderen added a comment -

Ah, crap. Somehow, I missed the e-mail notification of you assigning this issue back to me. I didn't see it before I released 1.0 yesterday night, sorry for that.

Anyway, the patch looks good. I've reviewed your patch and committed it to SVN, after doing a couple of minor updates. There's one thing that I'd like you to keep in mind when you're writing your next patch: Unit tests should ideally be very small. They should test not more than one unit of work. It is much clearer what goes wrong, if the test that fails just checks for one thing. Also, maintaining the test code is a lot easier if you keep things small and organized. A rule of thumb: if you can split up your unit test into more than one test, seriously consider doing so. I've split your original testValidBehavior up into multiple, distinct tests to reflect this.

Show
Guus der Kinderen added a comment - Ah, crap. Somehow, I missed the e-mail notification of you assigning this issue back to me. I didn't see it before I released 1.0 yesterday night, sorry for that. Anyway, the patch looks good. I've reviewed your patch and committed it to SVN, after doing a couple of minor updates. There's one thing that I'd like you to keep in mind when you're writing your next patch: Unit tests should ideally be very small. They should test not more than one unit of work. It is much clearer what goes wrong, if the test that fails just checks for one thing. Also, maintaining the test code is a lot easier if you keep things small and organized. A rule of thumb: if you can split up your unit test into more than one test, seriously consider doing so. I've split your original testValidBehavior up into multiple, distinct tests to reflect this.
Hide
Permalink
Guenther Niess added a comment -

Thanks for your advice, it was one of the first unit tests I wrote. I'll keep it in mind.

Show
Guenther Niess added a comment - Thanks for your advice, it was one of the first unit tests I wrote. I'll keep it in mind.

People

Vote (1)
Watch (2)

Dates

  • Created:
    Updated:
    Resolved:
Atlassian JIRA (v4.4.3#663-r165197) | Report a problem
Powered by a free Atlassian JIRA open source license for igniterealtime.org. Try JIRA - bug tracking software for your team.