Ranter
Join devRant
Do all the things like
++ or -- rants, post your own rants, comment on others' rants and build your customized dev avatar
Sign Up
Pipeless API
From the creators of devRant, Pipeless lets you power real-time personalized recommendations and activity feeds using a simple API
Learn More
Comments
-
Voxera115854yIt 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. -
-pthread4404yWell self.assertEqual should be self.assertTrue and why there is flag variable for everything.
flag = False
If f():
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-user13524yI found a lot of dependency in the code.
It use "with self.assertLogs().... as cm", that "cm" have "cm.output[0]" 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[0]. If we decided to store use other prefix , the test will fail. It will be great if cm.output[0] return empty string if error occur so we can just used
"if len(cm.output[0])>0"
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. -
mr-user13524y
-
-pthread4404yMy 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. -
-pthread4404y@mr-user during test setup, one can create temp directory and change current working directory to that temp directory so any file generated by output_engine will be dumped into that temp directory and in teardown we can just remove that temp dir.
-
-pthread4404y@mr-user I know it's bad to assert logging messages if it was upto me I just test original functionality that file is being generated.
-
@-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. -
@jespersh Well, Ex. The if os.path.isfile() line should not be an if statement, it should just assign the contains_filename variable.
Related Rants
Today, I found this in our upstream repo. How the fuck does it even get merged?
No use of tempfile, instead of idiomatic it's idiotic. I have changed that system, now I am hoping i don't have to refactor thia shit.
rant
shitty code
test