JMSLab / Template

Template for research repository using scons.
9 stars 1 forks source link

Add and refine unit tests for autofill functions #55

Closed jinfu93 closed 2 years ago

jinfu93 commented 2 years ago

Porting over from the sketch in #51 below are the additional tests we would like to implement in test_autofill.py:

@santiagohermo do feel free to add to this list or expand on the details of each test, thanks a lot!

santiagohermo commented 2 years ago

@santiagohermo do feel free to add to this list or expand on the details of each test, thanks a lot!

Thanks @jinfu93! I made a couple of edits to the opening comment.

jinfu93 commented 2 years ago

@santiagohermo I've added drafts of 2 tests: https://github.com/JMSLab/Template/blob/442eb18e64e6348e8768987c238b150b9140c874/source/lib/JMSLab/tests/test_autofill.py#L26-L44

Let me know if these look different from what you had in mind!

Regarding:

Make sure the behavior is as expected when using a string for format and a list with different formats.

I'm thinking of adding error handling to accommodate when autofill_formats doesn't line up with autofill_lists i.e. if autofill_lists is a nested list but autofill_formats is a string. Subsequently, the test can ensure the exception is thrown as expected. Let me know if you were thinking of something more straightforward than that.

I've also added a default for autofill_formats in 1a52e3e as you suggested.

Thanks!

jinfu93 commented 2 years ago

@santiagohermo implemented the following:

I'm thinking of adding error handling to accommodate when autofill_formats doesn't line up with autofill_lists i.e. if autofill_lists is a nested list but autofill_formats is a string. Subsequently, the test can ensure the exception is thrown as expected. Let me know if you were thinking of something more straightforward than that.

in a5e665e and 55c431a fyi. Thanks again!

santiagohermo commented 2 years ago

Thanks @jinfu93! On a first look the scripts look great. One small comment is that I'd try to not use sys.path.append to import files in upstream folders. I'd use the approach here instead.

Feel free to move to PR and I'll make a more detailed review there.

jinfu93 commented 2 years ago

Thanks @santiagohermo for the great tip! I attempted implementing the approach you suggested locally but was met with:

from ..autofill import GenerateAutofillMacros ImportError: attempted relative import with no known parent package

I also tried running locally the script source/lib/JMSLab/tests/test_build_lyx.py that you referenced but got the same error. I'm wondering if you might have some idea about whether that's a potential setup/configuration issue on my end? Thanks!

santiagohermo commented 2 years ago

Thanks @jinfu93! What was the working directory when you ran the script?

jinfu93 commented 2 years ago

@santiagohermo I tried running it from both the root of Template and from the location of the script.

santiagohermo commented 2 years ago

Can you try running it from source/lib/JMSLab? When I run pytest from that folder things seem to work fine.

jinfu93 commented 2 years ago

Thanks @santiagohermo! I could run pytest from sources/lib/JMSLab but running the script itself without pytest would still throw out the same error. Nonetheless, I could do some local testing with pytest which is more than sufficient. Thanks again!

jinfu93 commented 2 years ago

This thread continues in its PR #57.

jinfu93 commented 2 years ago

Summary:

In this issue, we completed the suite of unit tests for the new autofill function in Python along with its accompanying wrapper function.

Changes were merged into main in 35b4fef.

Final state of issue branch can be viewed here.