[asterisk-dev] Change in testsuite[master]: stasis: set a channel variable on websocket disconnect error

[Ashley Sanders (Code Review)]

Ashley Sanders has posted comments on this change.

Change subject: stasis: set a channel variable on websocket disconnect error
......................................................................


Patch Set 1:

(5 comments)

https://gerrit.asterisk.org/#/c/18/1/tests/rest_api/applications/stasisstatus/observable_object.py
File tests/rest_api/applications/stasisstatus/observable_object.py:

Line 122:         if self.__registrar[event] is None:
        :             msg += 'Instantiating the observers for event {0}.'.format(event)
        :             LOGGER.debug(msg)
        :             self.__registrar[event] = list()
> I may be misinterpreting your intent here, but I don't think this is going 
The code says:
Ask the registrar for the list of observers for event 'foo'. 
If the list is none, create an empty list.

If the event in question is not present in the dictionary, I want this to blow up. It needs to fail fast, to give the author of the test immediate and obnoxious feedback that they have written their test in an unsupported way.

If you refer to my original review draft: https://reviewboard.asterisk.org/r/4520/, you will see at least 2 issues raised by Matt Jordan that lean towards this same behavioral pattern.

---mjordan comment begin---
"This feels like it would be a test writing error, which means I would expect things to blow up (so that, as a test writer, I can fix them). Right now, if this just returns, test execution will hum along without actually indicating (other than as a WARNING) that something really bad just happened.

This finding applies to the other very off nominal paths in this class, particularly those where methods are passed invalid parameters."
---mjordan comment end---


Line 132:         if self.__suspended < 0:
        :             self.__suspended = 0
> Correct, but my point was that if self.__suspended is 0 when entering this 
Oh.my.gosh. My apologies. The tool made it difficult for me to see the full block of code in question due to the comment in the middle. My previous response was totally based on observing lines 132-133. 

I agree with you and I will correct this for clarity.

As a note, on line 188, you will notice that the only real criterion for "is this guy suspended?" => "is the value of self.__suspended > 0? If so, then yes, this guy is suspended. If not, then no, he is not suspended." 

Having said this, though, the check at line 132 _is not redundant_ and is very much necessary to ensure that the suspended flag does not get skewed by someone being overly defensive and calling resume too many times.


https://gerrit.asterisk.org/#/c/18/1/tests/rest_api/applications/stasisstatus/test_case.py
File tests/rest_api/applications/stasisstatus/test_case.py:

Line 64:         TestCase.ami_connect(self, ami)
> Calling TestCase.ami_connect() isn't necessary since ami_connect is a virtu
In Python, all methods are effectively virtual: https://docs.python.org/3/tutorial/classes.html#inheritance

"Derived classes may override methods of their base classes. Because methods have no special privileges when calling other methods of the same object, a method of a base class that calls another method defined in the same base class may end up calling a method of a derived class that overrides it. (For C++ programmers: all methods in Python are effectively virtual.)

An overriding method in a derived class may in fact want to extend rather than simply replace the base class method of the same name. There is a simple way to call the base class method directly: just call BaseClassName.methodname(self, arguments). This is occasionally useful to clients as well. (Note that this only works if the base class is accessible as BaseClassName in the global scope.)"

Essentially, this is a means of ensuring the logic supplied in the base class is executed + any custom logic supplied in the derived type.

If you search our testsuite, you will see that this same strategy is used in about 16 other places.


Line 126:         TestCase.run(self)
        : 
        :         self.create_ami_factory()
> I think that TestCase.run(self) will already create the AMI factory, so you
Here is the definition of TestCase.run:
------------------------------------------------------
def run(self):
        """Base implementation of the test execution method, run. Derived
        classes should override this and start their Asterisk dependent logic
        from this method.

        Derived classes must call this implementation, as this method provides a
        fail out mechanism in case the test hangs.
        """
        if self.reactor_timeout > 0:
            self.timeout_id = reactor.callLater(self.reactor_timeout,
                                                self._reactor_timeout)
------------------------------------------------------

I am following the precedent strategy. (A quick search of our testsuite produces roughly 196 matches following this same pattern.)


Line 142:         TestCase.stop_reactor(self)
> This notation is a bit odd. Why not just self.stop_reactor()?
Calling self.stop_reactor from self.stop_reactor will result in the following Python error:
RuntimeError: maximum recursion depth exceeded

As commented earlier, calling the base type method from within the derived type's overridden method is a means of extending the base type's logic, rather than outright overriding it.


-- 
To view, visit https://gerrit.asterisk.org/18
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I0f7dadfd429bd30e9f07a531f47884d8c923fc13
Gerrit-PatchSet: 1
Gerrit-Project: testsuite
Gerrit-Branch: master
Gerrit-Owner: Ashley Sanders <asanders at digium.com>
Gerrit-Reviewer: Ashley Sanders <asanders at digium.com>
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
Gerrit-HasComments: Yes