Update of base.Role + tests #112
Conversation
|
There's an error in the build. Can you run it and fix what's wrong? If in doubt, refer to here: https://provy.readthedocs.org/en/latest/contributing.html#how-to-develop |
|
You mean the: ./provy/core/roles.py:485: W801 redefinition of unused 'dirname' from line I have looked into it, and code is legit, it is a flake8 glitch. I have no idea what to do with it. 2013/1/30 Diogo Baeder notifications@github.com
|
|
OK error suppressed. |
| @@ -189,7 +196,11 @@ class MySampleRole(Role): | |||
| def cleanup(self): | |||
| pass | |||
| ''' | |||
| pass | |||
| for path in self._paths_to_remove: | |||
There was a problem hiding this comment.
This is interesting. There's a problem, though: every Role subclass expected the cleanup() method to not do anything, so they don't call super() to cleanup.
So, two things have to change:
- You must call cleanup() from super() in all roles that overwrite it;
- You need to change the docstring, because as it is now it doesn't seem to me that I should be careful about this method and remember to call it.
There was a problem hiding this comment.
There is other solution that wouldn't break existing code:
- add method
base_cleanupand call that in addition tocleanupwhen cleaning up after provyfile. - When calling cleanup check whether it called
Role.cleanupand if not log warning.
There was a problem hiding this comment.
Nevertheless i called supper in all all Roles I found.
| self.role.execute_python_script("script", False, False) | ||
|
|
||
| self.assertTrue(isinstance(values['put_file'].mock_calls[0][1][0], StringIO)) | ||
| @istest |
There was a problem hiding this comment.
Lacking a line space between this test case and the previous test. Are you running "make build" when changing your code?
There was a problem hiding this comment.
Most of the time yes.
Nevertheless fixed it.
Update of base.Role + tests
|
Looks good enough now. Some things to fix, yet, but we can go on improving it. |
Should be OK now.