-
Notifications
You must be signed in to change notification settings - Fork 272
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove redundant temp folder and unused imports #1224
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for taking the time to clean this up. I have a few minor comments, mostly on not adding semi-redundant comments to the code.
847f30c
to
28a1da5
Compare
Addressed all of @joshuagl comments and:
|
It seems like because we are supporting |
Ah, yes. The syntax changed for |
Would be good to get this cleanup landed. @MVrachev could you revert back to |
I'm a bit worried about
So I wonder if this should only be done if we also start creating the temp dirs somewhere more reasonable? Does anyone know when the temp dirs are not being removed? |
Besides the expected case when |
I would vote for that if possible. Polluting the tests source code dir on a failure is not particularly harmful but ... unpleasant |
Yes, the side effect is that it will create one temp dir per class instance.
As I understand it, the idea is to have those temp files available for you on test failure for analysis.
The doc says:
Again from the doc for tearDownClass:
Also again in the doc
|
Python has a function to get an OS appropriate, administrator configurable, temporary directory |
Sorry for giving you a bit random hints and not direct answers but in the Windows case temporary files created by But this is a Windows VM so it can be something specific to this case ... |
Blocked, until we remove Python2 from our tests as Joshua has mentioned. |
In many of the tests classes, we are creating two temporary directories: one with the name "temp_<random_string>" and inside it, we generate a new directory for each test with the name "Test<Class_Name>_<random_string>". I think we don't need the "temp_<random_string>" directory. The only benefit I can think of is that it could contain multiple temp folders from failing tests from a particular test run. But even then, this is not a big bonus because the name "temp_<random_string>" is not really descriptive from which test run this directory was created. Also, fixed the way we import "unittest_toolbox" and replace "unittest_toolbox.Modified_TestCase.function(self)" with "super().function()". PS: Thanks to Jussi Kukkonen who noticed we are using two temp folders per class in our tests. Signed-off-by: Martin Vrachev <[email protected]>
After the removal of python2 test cases in 13b0857 I rebased and updated this pr. |
Honestly, I am wondering about this change. |
I might have broken this PR quite badly with a recent bug fix (moved chained up teardown calls to the correct place in #1346 to fix issues found in another PR). Sorry about that -- I did not remember this one. I think that PR also makes this one a bit less needed: at least in the tests that I touched I made sure that we only try to remove the "topmost" directory (because that will remove every other directory inside it). In some cases there might still be unneeded temp directories. Let's discuss when you are back. |
As I said before, I don't feel so confident about this change anymore. |
Description of the changes being introduced by the pull request:
In many of the tests classes, we are creating two
temporary directories: one with the name "temp_<random_string>"
and inside it, we generate a new directory for each test with the name
Test<Class_Name>_<random_string>
.I think we don't need the "temp_<random_string>" directory.
The only benefit I can think of is that it could contain multiple
temp folders from failing tests from a particular test run.
But even then, this is not a big bonus because the name
temp_<random_string>
is not really descriptive from which testrun this directory was created.
PS: Thanks to Jussi Kukkonen who noticed we are using two temp
folders per class in our tests.
Signed-off-by: Martin Vrachev [email protected]
Please verify and check that the pull request fulfills the following
requirements: