Tickets/dm 10767 #59
Tickets/dm 10767 #59
Conversation
tests/testPosixStorage.py
Outdated
| fileType = file | ||
| except NameError: | ||
| from io import IOBase | ||
| fileType = IOBase |
There was a problem hiding this comment.
Can you use use io.IOBase in in Python 2? It is part of the standard library. If it works, I think using it would be a lot cleaner than the above. Just import it and use it.
If that does not work for some reason, then I think this would be significantly cleaner if your new code was put with the other import statements at the top of the file.
There was a problem hiding this comment.
file does not inherit from io.IOBase in python 2.
There was a problem hiding this comment.
Also, if you do need to still assign the class to a variable, please use a variable name that starts with a capital letter.
There was a problem hiding this comment.
+1 on putting it in the imports though
There was a problem hiding this comment.
I'm curious whether it matters that file inherits from io.IOBase. I realize that it may mean a tiny change to one test, but is that a bad thing?
tests/butlerProxy.py
Outdated
| if os.path.exists(os.path.join(ROOT, 'root/out')): | ||
| shutil.rmtree(os.path.join(ROOT, 'root/out')) | ||
| if os.path.exists(self.outputDir): | ||
| shutil.rmtree(self.outputDir) |
There was a problem hiding this comment.
Is it safe to not manually delete the butler? If it holds onto any objects that inherit from Citizen then failure to do so will trigger an error in the memory test.
There was a problem hiding this comment.
It seems risky to have setUp call tearDown, since normally users are free to add things to tearDown without worrying about breaking other methods such as setUp. Instead, I suggest that you make a new method deleteOutputDir that does what tearDown does above, then have setUp and tearDown call that.
Note that it would then be safe to have tearDown delete the butler again, which demonstrates my point.
There was a problem hiding this comment.
del doesn't actually do anything to destroy the object, it makes it available for garbage collection. when the test class is destroyed, the butler goes out of scope, same as if del had been called.
(and, the memory tests pass.)
There was a problem hiding this comment.
Yes the memory test passes now, but if butler ever is modified to include an object that inherits from Citizen it will stop passing and folks may be surprised at this. An explicit delete is safer.
There was a problem hiding this comment.
In any case I think refactoring the code as suggested is significantly cleaner.
There was a problem hiding this comment.
I don't see how allowing the butler instance to be GC'd will not delete a contained Citizen object whereas explicitly handing the butler to the GC will delete the contained Citizen...?
There was a problem hiding this comment.
Just to clarify: it is definitely true that we must explicitly delete test case attributes that inherit from Citizen in tearDown. If we don't, the memory test fails. Allowing such objects to be garbage collected definitely does not work. It is not a question of whether the objects eventually go out of scope -- of course they do -- it's whether they go out of scope fast enough that the memory test doesn't think memory has leaked.
Also, I thought CPython did delete immediately when the ref count dropped to 0 (unless there are cycles, which take longer to clean up?) -- though I agree the language specification does not require that. If this was not the case, then I'm surprised our memory test is usable; I would expect random failures in most of our packages.
There was a problem hiding this comment.
I suspect that the test case instance is not deleted until after the memory test has run, so by then it's too late for automatic garbage collection to help us. (I can't think why else we'd have to explicitly delete test case instances to avoid false memory leak errors).
tests/testPosixStorage.py
Outdated
| from io import IOBase | ||
| fileType = IOBase | ||
|
|
||
| isinstance(f, fileType) |
There was a problem hiding this comment.
You can probably get rid of this line, since the next line does the assert
|
I think that the test object remains in scope, so the GC doesn't happen until the process exists. |
|
so @r-owen is right, and the memory test fails. It does seem to be timing or platform related (passed on centos-6, failed on centos-7). |
No description provided.