Do all the things like ++ or -- rants, post your own rants, comment on others' rants and build your customized dev avatarSign Up
jespersh738341dWhat is shitty about it?
Voxera783741dIt looks like its a test to see if it can write a file and clean up afterwards.
But I also cannot spot the big error.
Well self.assertEqual should be self.assertTrue and why there is flag variable for everything.
flag = False
flag = True
Can be rewritten as flag = f()
And here he is creating file for testing in project directory instead of temporary directory so if some test fail before it hits os.remove it won't do clean up. So, one has to clean that dump by himself.
mr-user117341dI found a lot of dependency in the code.
It use "with self.assertLogs().... as cm", that "cm" have "cm.output" which store the file name.
It will be great if the output file name is unique every time otherwise it will be difficult to know why test fail since the condition may be true due to other "test".
There is also subtle problem in that "Output stored at" should be prefix of cm.output. If we decided to store use other prefix , the test will fail. It will be great if cm.output return empty string if error occur so we can just used
The main problem I have is that the "assertLogs() cm" and "output engine" are too connected in a way which make testing difficult.
In conclusion , we just need to know what @-pthread have problem with it.
Edited : Forget that I said anything , he posted his problem above.
My main problem is with clean up logic. Here, if logic for output_file raise some unexpected error or assertion fail then os remove won't get to run and an extra file will get generated.
Always this kind of logic should be in teardown.
jespersh738341dAlright fair enough. It has some faults, fix it and move on.
Asked since I am not very familiar with Python
mellowmymind3540dYou must be a delight to work with.
mellowmymind3540d@-pthread I love how you are focusing first and foremost on not using the utility assert.True function, and about how the Boolean variables assigning is not optimized. Then comes the REAL criticism.
This is a surefire way to get the message lost in a code review. Very non-senior of you.