[asterisk-dev] [Code Review]: Reimplement 'skip' property for testsuite

[Paul Belanger]

> On April 13, 2012, 10:37 a.m., Matt Jordan wrote:
> > asterisk/trunk/lib/python/asterisk/TestConfig.py, lines 87-97
> > <https://reviewboard.asterisk.org/r/1796/diff/2/?file=27204#file27204line87>
> >
> >     I'd still prefer this to be a functor, since that's essentially what it is.
> >     
> >     As an aside, I'm not sure we only want to be able to skip branches.  Its very conceivable that a test would be broken in a particular tag, in which case we'd want something like:
> >     
> >     min: 1.8
> >     skip:
> >       - tag: 1.8.10.1
> >       - tag: 10.4.1
> >     
> >     That is a separate use case then what this particular review was attempting to accomplish, but it is something we may want to add.

Converting this to a functor is fine however this is how we do it for a Dependency.  


> On April 13, 2012, 10:37 a.m., Matt Jordan wrote:
> > asterisk/trunk/lib/python/asterisk/TestConfig.py, line 96
> > <https://reviewboard.asterisk.org/r/1796/diff/2/?file=27204#file27204line96>
> >
> >     This logic feels like it would be better placed in the AsteriskVersion class itself.  Something like:
> >     
> >     if ast_version.is_same_branch(version):
> >     
> >     The fact that we have to check both the branch and the phone properties to determine that would be better handled by the class that maintains that logic.

will move.


> On April 13, 2012, 10:37 a.m., Matt Jordan wrote:
> > asterisk/trunk/lib/python/asterisk/TestConfig.py, lines 440-443
> > <https://reviewboard.asterisk.org/r/1796/diff/2/?file=27204#file27204line440>
> >
> >     This can be replaced with a list comprehension.
> >     
> >     self.can_run = len([s for s in self.skips where s.met is False] == 0)
> >     
> >     If any s.met is False, the result of the list comprehension will have the elements of s placed into it.  If any element of s exists in the list, we shouldn't run.
> 
> Matt Jordan wrote:
>     That actually should read:
>     
>     For any s where s.met is False, object s will be placed into the list built by the list comprehension.  Since we don't want to run if any object s is not met, then if the length of the resulting list is not 0, we shouldn't run.

Will try this out.


> On April 13, 2012, 10:37 a.m., Matt Jordan wrote:
> > asterisk/trunk/lib/python/asterisk/version.py, lines 253-259
> > <https://reviewboard.asterisk.org/r/1796/diff/2/?file=27205#file27205line253>
> >
> >     This could use some more unit tests.  Things the unit tests should confirm:
> >     
> >     1. Need to confirm that we can parse the 10-digiumphones branch
> >     2. What do we do if the phonesbranch doesn't have a parent branch?  Currently, the 10-digiumphones branch does not, as it hasn't had anything merged into it.
> >     3. How do we compare 1.8 to 1.8-digiumphones?  Currently, this treats 1.8-digiumphones as if it were 1.8.  That would imply that the comparison would rely on subversion revision numbers to determine if something is less than or greater than another branch.  Should the 1.8 branch ever be greater then 1.8-digiumphones?
> >

1) Adding, since 10-digiumphones didn't exist when I started.
2) Not sure I understand
3) That is the $100 question, which branch is greater?  I mentioned before, I think they are parallel branches, meaning they should not be compared.  Will need some feedback for this.


- Paul


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/1796/#review5988
-----------------------------------------------------------


On April 12, 2012, 10:27 a.m., Paul Belanger wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1796/
> -----------------------------------------------------------
> 
> (Updated April 12, 2012, 10:27 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> I'm looking for some initial feedback on the following patch.  Since we have the digiumphones branch now, we need to rework on the testsuite handles asterisk versions.  Before we used minversion / maxversion (which worked across branches) however this does not work well if you have a parallel branch.
> 
> So, I've removed them in favour of 'skip'.  Basically, the testsuite will run on every version of asterisk unless you have 'skip' defined, then depending on the flag (right now branch) it checks to run or skip the test.
> 
> We currently use the 'skip' flag, but only for a basic check.  At the moment to skip broken tests, this new method would give us more control on the type of tests to skip.  EG: Skip if OS is FreeBSD or skip if arch is i386.
> 
> 
> Diffs
> -----
> 
>   asterisk/trunk/lib/python/asterisk/TestConfig.py 3176 
>   asterisk/trunk/lib/python/asterisk/version.py 3176 
>   asterisk/trunk/runtests.py 3176 
>   asterisk/trunk/tests/channels/SIP/message_auth/test-config.yaml 3176 
>   asterisk/trunk/tests/channels/SIP/message_unauth/test-config.yaml 3176 
> 
> Diff: https://reviewboard.asterisk.org/r/1796/diff
> 
> 
> Testing
> -------
> 
> Local dev box.
> 
> There is also more work needed to be done in updating the test-config.yaml files, however I want to get this patch reviewed before making all those changes.
> 
> 
> Thanks,
> 
> Paul
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20120419/967b9f7c/attachment-0001.htm>