<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/4080/">https://reviewboard.asterisk.org/r/4080/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On October 15th, 2014, 4:43 p.m. CDT, <b>Matt Jordan</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://reviewboard.asterisk.org/r/4080/diff/1/?file=68347#file68347line82" style="color: black; font-weight: bold; text-decoration: underline;">/asterisk/trunk/runtests.py</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">def run(self):</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">82</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="p">(</span><span class="n">p</span><span class="o">.</span><span class="n">returncode</span> <span class="o">==</span> <span class="mi">0</span> <span class="ow">and</span> <span class="bp">self</span><span class="o">.</span><span class="n">test_config</span><span class="o">.</span><span class="n">expect_pass</span><span class="p">)</span> <span class="ow">or</span></pre></td>
</tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">83</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="p">(</span><span class="n">p</span><span class="o">.</span><span class="n">returncode</span> <span class="o">!=</span> <span class="mi">0</span> <span class="ow">and</span> <span class="ow">not</span> <span class="bp">self</span><span class="o">.</span><span class="n">test_config</span><span class="o">.</span><span class="n">expect_pass</span><span class="p">))</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">While the extra parantheses are probably needed, generally, this doesn't feel like the pythonic way to write this (although the original code isn't either). You shouldn't have to explicitly test for 0 or not 0.
self.passed = ((p.returncode and self.test_config.expect_pass) or (not p.returncode and not self.test_config.expect_pass))
This will evaluate self.passed to a boolean still.</pre>
</blockquote>
<p>On October 15th, 2014, 6:03 p.m. CDT, <b>jbigelow</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">test_runner.py returns 0 if passed and 1 if failed. So the above suggestion would need to be like so:
self.passed = ((not p.returncode and self.test_config.expect_pass) or (p.returncode and not self.test_config.expect_pass))
However it's still possible for self.passed to be an INT. Maybe I've gone crazy but the below examples say I'm not.
Example of not explicitly testing for 0 or not 0 as suggested:
>>> for returncode in (0, 1):
... for expect_pass in (True, False):
... ((not returncode and expect_pass) or (returncode and not expect_pass))
...
True
0
False
True
Example of explicitly testing for 0 and not 0 as posted for review:
>>> for returncode in (0, 1):
... for expect_pass in (True, False):
... ((returncode == 0 and expect_pass) or (returncode != 0 and not expect_pass))
...
True
False
False
True
</pre>
</blockquote>
<p>On October 16th, 2014, 6:08 p.m. CDT, <b>Scott Griepentrog</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I did not notice this exchange before my review. Normally open issue sections are not collapsed, but that wasn't the case?
My testing of the current patch showed correct function. After reviewing this discussion, I would suggest sanitizing p.returncode first to simplify the comparison thusly:
>>> for returncode in (0,1):
... for expect_pass in (True, False):
... did_pass = (returncode == 0)
... print returncode, expect_pass, did_pass, did_pass == expect_pass
...
0 True True True
0 False True False
1 True False False
1 False False True
resulting in:
did_pass = (p.returncode == 0)
self.passed = (did_pass == expect_pass)
</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Implemented Scott Griepentrog's suggestion.</pre>
<br />
<p>- jbigelow</p>
<br />
<p>On October 20th, 2014, 11:38 a.m. CDT, jbigelow 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 jbigelow.</div>
<p style="color: grey;"><i>Updated Oct. 20, 2014, 11:38 a.m.</i></p>
<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;">When the 'expected-result' (or 'expectedResult') YAML property for test configuration is set to False and the test fails, the test should be marked as passed. However it is marked as failed. This patch should fix the issue so that tests are marked as passed in this scenario.
Additionally:
* Check if p.returncode is not zero so self.passed is a boolean rather than an int in some cases.
* Added some print statements to make it clear why a test was marked as passed or failed when the 'expected-result' YAML property is set to False.
* Added text to the failure message so it's easily known when looking at the results file that the test was expected to fail but passed and therefore marked as failed.</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;">Tested the various scenarios and they all seem to properly work as expected now.</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/runtests.py <span style="color: grey">(5726)</span></li>
<li>/asterisk/trunk/lib/python/asterisk/test_config.py <span style="color: grey">(5726)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/4080/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>