[asterisk-dev] [Code Review]: Reimplement 'skip' property for testsuite
Matt Jordan
reviewboard at asterisk.org
Fri Apr 20 10:15:10 CDT 2012
> 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?
> >
>
> Paul Belanger wrote:
> 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.
>
> opticron wrote:
> 2) If you build 10-digiumphones right now, the version string you get is "SVN-branch-10-digiumphones-r362674". There is no parent identifier after the revision.
> 3) I agree that they should not be compared. I would expect minversion to operate only on the numeric portion of the branch. This should allow existing testsuite tests to work against both 1.8 and 1.8-digiumphones (or the 10 variants for that matter) without modification and future tests can exclude the base branches or digiumphones branches as necessary.
They're parallel...ish. Comparing versions between 1.8-digiumphones and 1.8 is inevitable, hence why the AsteriskVersion module needs to be able to handle it in a consistent manner. Consider the following scenario:
Say for example we have a test written against a regression caused in 1.8.11.0. Say that test gets a tag of 1.8.12.1, as we don't detect it until 1.8.12.0 is released and that is the minimum version of 1.8 needed to run the test. A current SVN checkout of 1.8 would also run the test, as an SVN revision is always considered to be greater then an actual tag. (Note that the tag min version is still useful, however, for when we test releases and release candidates). 1.8-digiumphones is locked (currently) at a previous version of 1.8 (approximately 1.8.11.0 with some other stuff). Because a minimum version of 1.8.12.1 is needed to run, and that evaluates to being less then an SVN revision number, the test would execute against 1.8-digiumphones and potentially fail.
This can be solved by telling the test to be skipped for 1.8-digiumphones, and removing that skip once 1.8-digiumphones is rebased to a more recent 1.8 version. This requires someone to remember to remove the skip property.
This isn't bad, nor does it mean we need to necessarily change how this behaves. The point of having unit tests for this however is to (a) ensure that comparison behavior is consistent, and (b) document how xx-digiumphones interacts with a main branch in terms of comparisons.
All of this is a roundabout way of getting to the conclusion that having comparison unit tests between the xx-digiumphone branches and other tags/branches is a good addition.
> 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.
>
> Paul Belanger wrote:
> Converting this to a functor is fine however this is how we do it for a Dependency.
I won't belabor this any further, as its a nice to have. Doing this and using the result shouldn't be difficult when we have to display why we won't run a test, as there are various mechanisms to print out objects as well (overriding the __str__ method), but again, its not important at this point.
- Matt
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/1796/#review5988
-----------------------------------------------------------
On April 19, 2012, 11:11 a.m., Paul Belanger wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1796/
> -----------------------------------------------------------
>
> (Updated April 19, 2012, 11:11 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/20120420/ba344455/attachment-0001.htm>
More information about the asterisk-dev
mailing list