> On Oct. 15, 2014, 4:43 p.m., Matt Jordan wrote: > > /asterisk/trunk/runtests.py, lines 82-83 > > <https://reviewboard.asterisk.org/r/4080/diff/1/?file=68347#file68347line82> > > > > 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. > > jbigelow wrote: > 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 > > > > Scott Griepentrog wrote: > 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) >
Implemented Scott Griepentrog's suggestion. - jbigelow ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4080/#review13533 ----------------------------------------------------------- On Oct. 20, 2014, 11:38 a.m., jbigelow wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviewboard.asterisk.org/r/4080/ > ----------------------------------------------------------- > > (Updated Oct. 20, 2014, 11:38 a.m.) > > > Review request for Asterisk Developers. > > > Repository: testsuite > > > Description > ------- > > 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. > > > Diffs > ----- > > /asterisk/trunk/runtests.py 5726 > /asterisk/trunk/lib/python/asterisk/test_config.py 5726 > > Diff: https://reviewboard.asterisk.org/r/4080/diff/ > > > Testing > ------- > > Tested the various scenarios and they all seem to properly work as expected > now. > > > Thanks, > > jbigelow > >
-- _____________________________________________________________________ -- 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
