[prev in list] [next in list] [prev in thread] [next in thread]
List: asterisk-dev
Subject: Re: [asterisk-dev] [Code Review] 3490: Testsuite: Ensure that repeated device states and presence st
From: "Matt Jordan" <reviewboard () asterisk ! org>
Date: 2014-04-29 18:46:49
Message-ID: 20140429184649.1056.15165 () sonic ! digium ! api
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3490/#review11786
-----------------------------------------------------------
Ship it!
Ship It!
- Matt Jordan
On April 29, 2014, 9:20 a.m., Mark Michelson wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3490/
> -----------------------------------------------------------
>
> (Updated April 29, 2014, 9:20 a.m.)
>
>
> Review request for Asterisk Developers.
>
>
> Bugs: ASTERISK-23672
> https://issues.asterisk.org/jira/browse/ASTERISK-23672
>
>
> Repository: testsuite
>
>
> Description
> -------
>
> In ASTERISK-23672, it was reported that when the presence state of an entity was \
> changed such that the state was the same but the subtype or message differed from \
> what was previously set, NOTIFYs were not sent to SIP subscribers. Looking at the \
> code, it appeared that res_pjsip_exten_state was being overzealous in trying to \
> filter out repeated state changes and that the core PBX code already performed the \
> necessary filtering.
> I made the following change to res_pjsip_exten_state.c, which I'm not placing in \
> its own review due to its small size:
> Index: res/res_pjsip_exten_state.c
> ===================================================================
> --- res/res_pjsip_exten_state.c (revision 412578)
> +++ res/res_pjsip_exten_state.c (working copy)
> @@ -334,11 +334,6 @@
> struct notify_task_data *task_data;
> struct exten_state_subscription *exten_state_sub = data;
>
> - if (exten_state_sub->last_exten_state == info->exten_state &&
> - exten_state_sub->last_presence_state == info->presence_state) {
> - return 0;
> - }
> -
> if (!(task_data = alloc_notify_task_data(exten, exten_state_sub, info))) {
> return -1;
> }
>
> I then created three testsuite tests to ensure that behavior is as we expect for it \
> to be:
> devstate_repeat: This relies on the device state for a custom device state to be \
> "not in use" initially. A subscriber subscribes to a custom device state. We then \
> set a device state change for the custom device state to be "not in use". Since \
> there is no change in the device state, no NOTIFY should be sent to \
> the subscriber.
> presencestate_repeat: This sets an initial presence state for a CustomPresence \
> entity. A SIP subscriber subscribes to the custom presence state. We then set the \
> presence state to the exact same value again and ensure that no additional NOTIFYs \
> are sent to the subscriber.
> presencestate_repeat_okay: This sets an initial presence state for a CustomPresence \
> entity. A SIP subscriber subscribes to the custom presence state. We then change \
> the presence state twice. The first change keeps the same state and message and \
> changes the subtype. The second change keeps the same state and subtype and changes \
> the message. These should result in two additional NOTIFYs being sent to the \
> subscriber.
>
> Diffs
> -----
>
> /asterisk/trunk/tests/channels/pjsip/subscriptions/presence/tests.yaml 5004
> /asterisk/trunk/tests/channels/pjsip/subscriptions/presence/presencestate_repeat_okay/test-config.yaml \
> PRE-CREATION
> /asterisk/trunk/tests/channels/pjsip/subscriptions/presence/presencestate_repeat_okay/sipp/subscribe.xml \
> PRE-CREATION
> /asterisk/trunk/tests/channels/pjsip/subscriptions/presence/presencestate_repeat_okay/repeat_presence_state.py \
> PRE-CREATION
> /asterisk/trunk/tests/channels/pjsip/subscriptions/presence/presencestate_repeat_okay/configs/ast1/pjsip.conf \
> PRE-CREATION
> /asterisk/trunk/tests/channels/pjsip/subscriptions/presence/presencestate_repeat_okay/configs/ast1/extensions.conf \
> PRE-CREATION
> /asterisk/trunk/tests/channels/pjsip/subscriptions/presence/presencestate_repeat/test-config.yaml \
> PRE-CREATION
> /asterisk/trunk/tests/channels/pjsip/subscriptions/presence/presencestate_repeat/sipp/subscribe.xml \
> PRE-CREATION
> /asterisk/trunk/tests/channels/pjsip/subscriptions/presence/presencestate_repeat/repeat_presence_state.py \
> PRE-CREATION
> /asterisk/trunk/tests/channels/pjsip/subscriptions/presence/presencestate_repeat/configs/ast1/pjsip.conf \
> PRE-CREATION
> /asterisk/trunk/tests/channels/pjsip/subscriptions/presence/presencestate_repeat/configs/ast1/extensions.conf \
> PRE-CREATION
> /asterisk/trunk/tests/channels/pjsip/subscriptions/presence/devstate_repeat/test-config.yaml \
> PRE-CREATION
> /asterisk/trunk/tests/channels/pjsip/subscriptions/presence/devstate_repeat/sipp/subscribe.xml \
> PRE-CREATION
> /asterisk/trunk/tests/channels/pjsip/subscriptions/presence/devstate_repeat/repeat_device_state.py \
> PRE-CREATION
> /asterisk/trunk/tests/channels/pjsip/subscriptions/presence/devstate_repeat/configs/ast1/pjsip.conf \
> PRE-CREATION
> /asterisk/trunk/tests/channels/pjsip/subscriptions/presence/devstate_repeat/configs/ast1/extensions.conf \
> PRE-CREATION
> Diff: https://reviewboard.asterisk.org/r/3490/diff/
>
>
> Testing
> -------
>
> Prior to the diff mentioned in the description, devstate_repeat and \
> presencestate_repeat would pass, but presencestate_repeat_okay would not. With the \
> diff above applied, all three tests pass.
>
> Thanks,
>
> Mark Michelson
>
>
[Attachment #5 (text/html)]
<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 \
solid;"> <tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://reviewboard.asterisk.org/r/3490/">https://reviewboard.asterisk.org/r/3490/</a>
</td>
</tr>
</table>
<br />
<p>Ship it!</p>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Ship It!</pre> <br />
<p>- Matt Jordan</p>
<br />
<p>On April 29th, 2014, 9:20 a.m. CDT, Mark Michelson wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" \
style="background-image: \
url('https://reviewboard.asterisk.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); \
background-position: left top; background-repeat: repeat-x; border: 1px black \
solid;"> <tr>
<td>
<div>Review request for Asterisk Developers.</div>
<div>By Mark Michelson.</div>
<p style="color: grey;"><i>Updated April 29, 2014, 9:20 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://issues.asterisk.org/jira/browse/ASTERISK-23672">ASTERISK-23672</a>
</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
testsuite
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" \
style="border: 1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">In ASTERISK-23672, it was reported that when the presence state of an \
entity was changed such that the state was the same but the subtype or message \
differed from what was previously set, NOTIFYs were not sent to SIP subscribers. \
Looking at the code, it appeared that res_pjsip_exten_state was being overzealous in \
trying to filter out repeated state changes and that the core PBX code already \
performed the necessary filtering.
I made the following change to res_pjsip_exten_state.c, which I'm not placing in \
its own review due to its small size:
Index: res/res_pjsip_exten_state.c
===================================================================
--- res/res_pjsip_exten_state.c (revision 412578)
+++ res/res_pjsip_exten_state.c (working copy)
@@ -334,11 +334,6 @@
struct notify_task_data *task_data;
struct exten_state_subscription *exten_state_sub = data;
- if (exten_state_sub->last_exten_state == info->exten_state &&
- exten_state_sub->last_presence_state == info->presence_state) {
- return 0;
- }
-
if (!(task_data = alloc_notify_task_data(exten, exten_state_sub, info))) {
return -1;
}
I then created three testsuite tests to ensure that behavior is as we expect for it \
to be:
devstate_repeat: This relies on the device state for a custom device state to be \
"not in use" initially. A subscriber subscribes to a custom device state. \
We then set a device state change for the custom device state to be "not in \
use". Since there is no change in the device state, no NOTIFY should be sent to \
the subscriber.
presencestate_repeat: This sets an initial presence state for a CustomPresence \
entity. A SIP subscriber subscribes to the custom presence state. We then set the \
presence state to the exact same value again and ensure that no additional NOTIFYs \
are sent to the subscriber.
presencestate_repeat_okay: This sets an initial presence state for a CustomPresence \
entity. A SIP subscriber subscribes to the custom presence state. We then change the \
presence state twice. The first change keeps the same state and message and changes \
the subtype. The second change keeps the same state and subtype and changes the \
message. These should result in two additional NOTIFYs being sent to the \
subscriber.</pre> </td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: \
1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">Prior to the diff mentioned in the description, devstate_repeat and \
presencestate_repeat would pass, but presencestate_repeat_okay would not. With the \
diff above applied, all three tests pass.</pre> </td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>/asterisk/trunk/tests/channels/pjsip/subscriptions/presence/tests.yaml <span \
style="color: grey">(5004)</span></li>
<li>/asterisk/trunk/tests/channels/pjsip/subscriptions/presence/presencestate_repeat_okay/test-config.yaml \
<span style="color: grey">(PRE-CREATION)</span></li>
<li>/asterisk/trunk/tests/channels/pjsip/subscriptions/presence/presencestate_repeat_okay/sipp/subscribe.xml \
<span style="color: grey">(PRE-CREATION)</span></li>
<li>/asterisk/trunk/tests/channels/pjsip/subscriptions/presence/presencestate_repeat_okay/repeat_presence_state.py \
<span style="color: grey">(PRE-CREATION)</span></li>
<li>/asterisk/trunk/tests/channels/pjsip/subscriptions/presence/presencestate_repeat_okay/configs/ast1/pjsip.conf \
<span style="color: grey">(PRE-CREATION)</span></li>
<li>/asterisk/trunk/tests/channels/pjsip/subscriptions/presence/presencestate_repeat_okay/configs/ast1/extensions.conf \
<span style="color: grey">(PRE-CREATION)</span></li>
<li>/asterisk/trunk/tests/channels/pjsip/subscriptions/presence/presencestate_repeat/test-config.yaml \
<span style="color: grey">(PRE-CREATION)</span></li>
<li>/asterisk/trunk/tests/channels/pjsip/subscriptions/presence/presencestate_repeat/sipp/subscribe.xml \
<span style="color: grey">(PRE-CREATION)</span></li>
<li>/asterisk/trunk/tests/channels/pjsip/subscriptions/presence/presencestate_repeat/repeat_presence_state.py \
<span style="color: grey">(PRE-CREATION)</span></li>
<li>/asterisk/trunk/tests/channels/pjsip/subscriptions/presence/presencestate_repeat/configs/ast1/pjsip.conf \
<span style="color: grey">(PRE-CREATION)</span></li>
<li>/asterisk/trunk/tests/channels/pjsip/subscriptions/presence/presencestate_repeat/configs/ast1/extensions.conf \
<span style="color: grey">(PRE-CREATION)</span></li>
<li>/asterisk/trunk/tests/channels/pjsip/subscriptions/presence/devstate_repeat/test-config.yaml \
<span style="color: grey">(PRE-CREATION)</span></li>
<li>/asterisk/trunk/tests/channels/pjsip/subscriptions/presence/devstate_repeat/sipp/subscribe.xml \
<span style="color: grey">(PRE-CREATION)</span></li>
<li>/asterisk/trunk/tests/channels/pjsip/subscriptions/presence/devstate_repeat/repeat_device_state.py \
<span style="color: grey">(PRE-CREATION)</span></li>
<li>/asterisk/trunk/tests/channels/pjsip/subscriptions/presence/devstate_repeat/configs/ast1/pjsip.conf \
<span style="color: grey">(PRE-CREATION)</span></li>
<li>/asterisk/trunk/tests/channels/pjsip/subscriptions/presence/devstate_repeat/configs/ast1/extensions.conf \
<span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/3490/diff/" style="margin-left: \
3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>
--
_____________________________________________________________________
-- Bandwidth and Colocation Provided by http://www.api-digital.com --
asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
http://lists.digium.com/mailman/listinfo/asterisk-dev
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic