[asterisk-dev] [Code Review] Unit test for diaplan pattern matching

[Russell Bryant]

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


Nice work, Mark.  I'm looking forward to getting this test in and encouraging others to expand on the test cases!


/trunk/tests/test_pbx.c
<https://reviewboard.asterisk.org/r/504/#comment3397>

    Copyright (C) 2010, Digium, Inc.



/trunk/tests/test_pbx.c
<https://reviewboard.asterisk.org/r/504/#comment3404>

    "PBX tests" seems so generic.  pbx.h and pbx.c are so overloaded.  What if we just limit this as a module for extension matching specifically?  I imagine that there will be a _lot_ of test cases that we come up with that will justify this test living by itself.



/trunk/tests/test_pbx.c
<https://reviewboard.asterisk.org/r/504/#comment3405>

    replace the defaultenabled line with <depend>TEST_FRAMEWORK</depend>



/trunk/tests/test_pbx.c
<https://reviewboard.asterisk.org/r/504/#comment3398>

    You can doxygenize this comment, too



/trunk/tests/test_pbx.c
<https://reviewboard.asterisk.org/r/504/#comment3399>

    static const int



/trunk/tests/test_pbx.c
<https://reviewboard.asterisk.org/r/504/#comment3401>

    Instead of a separate dynamically allocated contexts array, you could redefine the context_strings table to be a contents table, where each row has both an ast_context * and the context name.



/trunk/tests/test_pbx.c
<https://reviewboard.asterisk.org/r/504/#comment3402>

    static const char registrar[], instead of *registrar.



/trunk/tests/test_pbx.c
<https://reviewboard.asterisk.org/r/504/#comment3400>

    In earlier sections, you refer to the context names as context_strings[index], but here you refer to them by name.  Between the two, I like this method as it is more clear without having to refer back to the context_strings table.
    
    I'd like to propose a 3rd alternative.  What if you define the following prior to defining your context strings table:
    
    static const char TEST_PATTERN[] = "test_pattern";
    static const char TEST_PATTERN_INCLUDE[] = "test_pattern_include";
    
    Then, refer to them by their constant name.  That way, it is very clear which context you are referring to, but you don't risk any errors due to typos that could happen if you use raw strings.



/trunk/tests/test_pbx.c
<https://reviewboard.asterisk.org/r/504/#comment3403>

    Use status_update() to indicate the reason for failure


- Russell


On 2010-02-14 20:04:15, Mark Michelson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/504/
> -----------------------------------------------------------
> 
> (Updated 2010-02-14 20:04:15)
> 
> 
> Review request for Asterisk Developers, Jared Smith and lmadsen.
> 
> 
> Summary
> -------
> 
> Here I present a test module for testing pattern matching in the dialplan.
> 
> This is a unit test that runs within Asterisk, so all of the test inputs come from the test code itself. The test is based on a series of arrays that define contexts to create, extensions to place into those contexts, and finally test patterns to run against the dialplan that gets built. The test is designed such that adding new contexts, extensions, or test patterns to the test is as simple as adding new rows of information to arrays. The comments in the test code should be clear enough to show how this is done.
> 
> Currently, the test does not do very much. Only two contexts and three extensions are registered, and four test patterns are attempted to be matched. Right now though, I'm more concerned with the validity of the design of the test itself, as well as its current method of execution. By placing a test with a solid foundation into subversion, even with few actual test cases, people will have easier access to add new test cases as necessary.
> 
> I have placed this test in a new module called tests/test_pbx.c. Within this module, I expect other tests to be implemented which pertain to the PBX core as well. Other suggestions that have arisen during discussions are tests that actually run a PBX on a channel so that it can be ensured that the priorities execute in the proper order and so that statements such as Goto go to the proper place depending on data at the time of its use.
> 
> 
> This addresses bug 16809.
>     https://issues.asterisk.org/view.php?id=16809
> 
> 
> Diffs
> -----
> 
>   /trunk/tests/test_pbx.c PRE-CREATION 
> 
> Diff: https://reviewboard.asterisk.org/r/504/diff
> 
> 
> Testing
> -------
> 
> Currently, the test builds the following dialplan (comments added for easy explanation of test pattern matching):
> 
> [test_pattern]
> 
> include => test_pattern_include
> 
> exten => _2.,1,Noop(_2.) ; line 1
> exten => 2000,1,Noop(2000) ; line 2
> 
> [test_pattern_include]
> 
> exten => 2000,2,Noop(2000) ; line 3
> 
> The following test patterns exist:
> 
> 1. exten: 200, context: test_pattern, priority: 1 ; matches line 1 as expected
> 2. exten: 2000, context: test_pattern, priority: 1 ; matches line 2 as expected
> 3. exten: 2000, context: test_pattern, priority: 2 ; matches line 3 as expected
> 4. exten: 2000, context: test_pattern_include, priority: 2 ; matches line 3 as expected
> 
> As stated earlier, this is by no means exhaustive. It is, however, a good starting point and should demonstrate what the test code is capable of. The only capability of the test code that is not demonstrated here is the use of CID matching.
> 
> 
> Thanks,
> 
> Mark
> 
>