Pull Request for #55: Add and refine unit tests for autofill functions

jinfu93 commented 2 years ago

@santiagohermo significant changes here are contained in source/lib/JMSLab/tests/test_autofill.py where we added 3 additional tests on top of checking for out file existence (as per this list).

Also added default format for autofill wrapper in source/lib/JMSLab/autofill.py.

Thanks!

jmshapir commented 2 years ago

@santiagohermo thanks in advance for your review of this pull!

santiagohermo commented 2 years ago

Thanks @santiagohermo @jinfu93! Task received, I'll get to work on it probably tomorrow. (I should have flagged my plans before, apologies!)

santiagohermo commented 2 years ago

Thanks for the replies @jinfu93! I resolved a bunch of comments and now only a few remain.

jinfu93 commented 2 years ago

Thanks @santiagohermo for iterating on the details carefully with me! Appreciate all the very helpful comments and tips. I think we might be done with the remaining open points on your review but do let me know if that's not the case.

Should we try to tackle the warnings as per your above comment as well? I was wondering if you have any ideas what might be causing them. Thanks!

santiagohermo commented 2 years ago

On the tests, from the screenshot here.

It looks like the first one comes from a package so not sure we can do a lot there.
The second one seems to be caused by this function, and in particular I would guess L144. I conjecture that changing the 1.0 to 1 in that line will remove the warning. However, we should make sure the 1.0 is not essential there.

jinfu93 commented 2 years ago

Thanks for investigating @santiagohermo! I do get the same warnings as well. I tried your suggested fix but it seems like 1.0 was essential for the test.

Should we dive more into test_log.py to investigate?

santiagohermo commented 2 years ago

Thanks @jinfu93! I made a little bit of digging on both warnings (flagged in https://github.com/JMSLab/Template/pull/57#pullrequestreview-943489048).

The first warning has been flagged in https://github.com/pyreadline/pyreadline/issues/65. I expect this to solve itself for future versions of this package.

As we discussed above, the second warning arises in line 144 of the below

https://github.com/JMSLab/Template/blob/fff134e5d649825bf81801c2a80e2acb8f8d2c0c/source/lib/JMSLab/tests/test_log.py#L130-L150

The reason that changing 1.0 for 1 doesn't work is related to the note in the function's description. I would recommend to just drop lines 143-144. An alternative would be to add some error handling in log.py for the cases when the argument log of start_log() is an integer or a boolean, but this may be overkill. What do you think? We can also ask Mauricio his opinion here.

jinfu93 commented 2 years ago

Thanks for digging into this @santiagohermo!

The first warning has been flagged in https://github.com/pyreadline/pyreadline/issues/65. I expect this to solve itself for future versions of this package.

This sounds good to me as well.

The reason that changing 1.0 for 1 doesn't work is related to the note in the function's description.

Great catch! I should've read the description myself earlier.

@mcaceresb would you happen to be familiar with the 2nd warning here? @santiagohermo made some suggestions above and we were wondering if you might have an idea of how best to proceed? Thanks a lot!

mcaceresb commented 2 years ago

@jinfu93 @santiagohermo The warnings should be fine. The first is a deprecation warning for a package that should be OK to use so long as it's upgraded whenever python is upgraded (i.e. if you go to Python 3.9+ you should also upgrade all packages).

The second refers to the fact you can pass an integer instead of a file name to open. One if the tests passes 1.0 or similar and expects it to fail since that's not a valid input. Python is saying that it can try to convert the float to an integer so that open doesn't fail but this behavior is deprecated. I included a note about it in the test description.

I don't think you can get rid of the first warning. I think you can get rid of the second by passing e.g. 1.1 instead of 1.0 to the log in the log test, but it shouldn't be necessary (since the test is supposed to fail).

jinfu93 commented 2 years ago

Thanks so much for your help looking into this @mcaceresb, that makes a lot of sense! I'm good to leave as is if you are @santiagohermo, in which case I think we can wrap up here?

@jmshapir feel free to let us know if you have any further thoughts before we conclude here.

Thanks everyone!

santiagohermo commented 2 years ago

Thanks for the comments @mcaceresb! I agree @jinfu93, let's wrap up here.

jinfu93 commented 2 years ago

Summary for associated issue can be viewed here.

JMSLab / Template

Pull Request for #55: Add and refine unit tests for autofill functions #57