[asterisk-dev] [Code Review] 3476: Memory Pre- and Post-Test Condition

Thu Apr 24 17:46:00 CDT 2014

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

I suggest writing a sample yaml file that illustrates how this is intended to be used and explains what the default values are for the various configuration options.

./asterisk/trunk/lib/python/asterisk/memory_test_condition.py
<https://reviewboard.asterisk.org/r/3476/#comment21511>

    memory_info is a list, not a dictionary.

./asterisk/trunk/lib/python/asterisk/memory_test_condition.py
<https://reviewboard.asterisk.org/r/3476/#comment21515>

    This doesn't do a good job of explaining what memory_range actually is intended to be used for.

./asterisk/trunk/lib/python/asterisk/memory_test_condition.py
<https://reviewboard.asterisk.org/r/3476/#comment21512>

    This can be trimmed down a bit. The .get method of dictionaries allows for you to specify a second argument to be used if the key cannot be found in the dictionary.

    self.memory_range = test_config.config.get('memoryrange', 7000000)
    self.allocations = test_config.config.get('allocations', [])
    self.both = test_config.config.get('both', False)

./asterisk/trunk/lib/python/asterisk/memory_test_condition.py
<https://reviewboard.asterisk.org/r/3476/#comment21516>

    Since you do not override add_build_option in your subclass of TestCondition, you don't need to use super. You can just call self.add_build_option() directly since the subclass inherits all methods from the base class.

./asterisk/trunk/lib/python/asterisk/memory_test_condition.py
<https://reviewboard.asterisk.org/r/3476/#comment21513>

    You can simply initialize this dictionary to have all the necessary values in it instead of adding them one-by-one:

    info = {
        'name': 'Global',
        'memory': memory,
        'range': self.memory_range
    }

./asterisk/trunk/lib/python/asterisk/memory_test_condition.py
<https://reviewboard.asterisk.org/r/3476/#comment21514>

    You can do a similar dictionary initialization here. Set tokens before initializing info:

    info = {
        'memory': int(tokens[0]),
        'name': allocation['name'],
        'range': allocation.get('range', 500)
    }

./asterisk/trunk/lib/python/asterisk/memory_test_condition.py
<https://reviewboard.asterisk.org/r/3476/#comment21517>

    Just call self.pass_check() instead of super.

./asterisk/trunk/lib/python/asterisk/memory_test_condition.py
<https://reviewboard.asterisk.org/r/3476/#comment21518>

    Just use self.get_memory() instead of super

./asterisk/trunk/lib/python/asterisk/memory_test_condition.py
<https://reviewboard.asterisk.org/r/3476/#comment21528>

    This seems like it's not going to work properly. related_test_condition.memory_info and self.memory_info are not guaranteed to have related memory allocations at the same ordinal positions in their lists. Also, it's possible for a requested allocation not to exist in related_test_condition.memory_info but to be present in self.memory_info.

    So I have two suggestions here. Change the  loop to iterate over self.memory_info instead of related_test_condition.memory_info. Get the precondition's memory information by key.

    Something like the following:

    for post_cond in self.memory_info:
        pre_cond = None
        for item in related_test_condition.memory_info:
            if post_cond['name'] == item['name']:
                pre_cond = item
                break

    Now you have your correlated pre and post memory counts for any given allocation. There may be some spiffier way of doing the inner loop, but I don't know it.

    Note that you may not find a corresponding pre_cond for a given post_cond. In this case, you'll have to assume that 0 bytes of memory had previously been allocated.

./asterisk/trunk/lib/python/asterisk/memory_test_condition.py
<https://reviewboard.asterisk.org/r/3476/#comment21526>

    I highly recommend using more descriptive names for these dictionaries than dictionary1 and dictionary2.

./asterisk/trunk/lib/python/asterisk/memory_test_condition.py
<https://reviewboard.asterisk.org/r/3476/#comment21529>

    I'm a bit torn on the use of fabs() here. This will currently cause tests to fail if the memory usage *decreases* during the test run by the configured range. On the one hand, if the memory used goes down by that much, that's kind of a good thing since we're cleaning up after ourselves. But on the other hand, if the memory is spiking by that much, it's possible that something bad is happening.

./asterisk/trunk/lib/python/asterisk/memory_test_condition.py
<https://reviewboard.asterisk.org/r/3476/#comment21519>

    Call self.fail_check() instead of super.

./asterisk/trunk/lib/python/asterisk/memory_test_condition.py
<https://reviewboard.asterisk.org/r/3476/#comment21520>

    Call self.fail_check() instead of super.

./asterisk/trunk/lib/python/asterisk/memory_test_condition.py
<https://reviewboard.asterisk.org/r/3476/#comment21521>

    Call self.pass_check() instead of super.

./asterisk/trunk/lib/python/asterisk/memory_test_condition.py
<https://reviewboard.asterisk.org/r/3476/#comment21522>

    Call self.get_memory() instead of super.

./asterisk/trunk/lib/python/asterisk/test_conditions.py
<https://reviewboard.asterisk.org/r/3476/#comment21530>

    Any particular reason you switched this away from raising an exception?

- Mark Michelson

On April 24, 2014, 7:02 p.m., Benjamin Keith Ford wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3476/
> -----------------------------------------------------------
> 
> (Updated April 24, 2014, 7:02 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-18429
>     https://issues.asterisk.org/jira/browse/ASTERISK-18429
> 
> 
> Repository: testsuite
> 
> 
> Description
> -------
> 
> This testcondition can be enabled for any test using the keyword 'memory' under testconditions. The purpose of this testcondition is to check the memory allocated before and after the test, and make sure they are within a certain range. If the test wants to look at something specific (such as channel.c), then each allocation that you want to look at can also be specified in under 'allocations'. If both the global memory and individual allocations are to be checked by the test, that option can be enabled by setting 'both' to value True.
> 
> 
> Diffs
> -----
> 
>   ./asterisk/trunk/test-config.yaml 4944 
>   ./asterisk/trunk/lib/python/asterisk/test_conditions.py 4944 
>   ./asterisk/trunk/lib/python/asterisk/test_case.py 4944 
>   ./asterisk/trunk/lib/python/asterisk/memory_test_condition.py PRE-CREATION 
> 
> Diff: https://reviewboard.asterisk.org/r/3476/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Benjamin Keith Ford
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140424/384a33d7/attachment-0001.html>