Discuss where to put helper function in python file, within run() or same level of run()?
Opened this issue · 14 comments
This issue comes from #3256 (comment).
I'd like to create this issue to specifically talk about this.
In the existing codes, some helper functions are defined within run() function, such as in libvirt/tests/src/usb_device.py, other helper function are defined at the same level of run() function, such as in libvirt/tests/src/multifunction.py. We do not have a strict/recommended rule for this in the past, so contributors wrote codes in their preferred way.
In PR 3256, @smitterl suggested we'd better use the latter way which is to define at same level of run() function because
a) avoid using variables that are not passed, b) improve readability
I hope we can reach an agreement in this issue, then the contributors will follow the conclusion in future pull requests.
Welcome to add your opinions. @chunfuwen @kylazhang @chloerh @Yingshun @yafu-1 @smitterl @kvarga, @fangge1212
Feel free to involve more people.
I like @Yingshun 's proposal to move them to other files. This way, it will also be easier to move them to avocado-vt if found useful for other test providers like tp-qemu. Regarding the location, I am not sure if the test scripts should be mixed with the helper function scripts in the same folder. In case of libvirt_version it's been placed in the "provider" module. https://github.com/autotest/tp-libvirt/tree/master/provider
Maybe all helper scripts could initially live in /tp-libvirt/helpers ? (If decided to do something like this, the provider module might be moved there, too, for consistency.)
@smitterl Sounds great, I agree with you, it would be better to put them in one place.
BTW, the provider.libvirt_version module is no longer used in our test scripts, we've moved it to avocado-vt, see virttest.libvirt_version.
We have one more module provider.v2v_vmcheck_helper, @xiaodwan , you are the main contributor of this module, would you like to share your opinion?
We'd better not export module under src folder of tp-libvirt as package to allow be imported by other test case module(as #3343), this may lead to module import relationship mess. In default, there is clear line that tp-libvirt hold test cases,which will import modules from avocado-vt or avocado.
@chunfuwen, Sebas and I have reached an agreement that we should put these helper functions to one place if we do so. But I'm not sure about the package's name.
I just found that tp-qemu has some utilities under the provider, so we may just as well have a try.
@chunfuwen, Sebas and I have reached an agreement that we should put these helper functions to one place if we do so. But I'm not sure about the package's name.
I just found that tp-qemu has some utilities under the provider, so we may just as well have a try.
1)One of potential issue is that it will largely increase risk of Circular Dependencies import if test case can import module in other test case folder
2)In tp-libvirt folder, consistently in code structure, we keep one pair that one cfg has one matching .py file
3)If one module import another in tp-libvirt, that means one test case can import another case implement, it will leads to dependence between test cases. This is against fundamental principal in case design
4)If some utils is put under tp-libvirt, that means avocado-vt will probably import them. it will introduce the circle : tp-libvirt--avocado-vt-tp-libvirt
I think what yingshun and sebas have reached the agreement is that it is ok to add common functions into the 'provider' or 'helper' folder in tp-libvirt. But in fact we made a decision that we should move shared functions into avocado-vt before. So it seems it is time to review the decision again to see if we need some adjustment.
If we use 'helper'/'provider' in tp-libvirt, we'd better think about how to determine what is the criteria for putting a function into 'helper'/'provider' or avocado-vt.
@kylazhang @chloerh @kamilvarga ,could you share your thoughts? Then we will make a decision.
I think what yingshun and sebas have reached the agreement is that it is ok to add common functions into the 'provider' or 'helper' folder in tp-libvirt. But in fact we made a decision that we should move shared functions into avocado-vt before. So it seems it is time to review the decision again to see if we need some adjustment.
If we use 'helper'/'provider' in tp-libvirt, we'd better think about how to determine what is the criteria for putting a function into 'helper'/'provider' or avocado-vt.
From my point of view, the criteria of moving function to avocado-vt is usage in more than one test. That means the function is effective enough. Anyway, this moving will require also updates in import section of tests that are using these functions (additional effort). Also, it will probably result in a LOT of new functions in avocado-vt so we should make sure, they are well documented and easy to find, when needed. So, we do not end up with a multiple functions with very same behavior. I think it would be a good to have one person, who will maintain this or some rules to follow.
@chunfuwen, Sebas and I have reached an agreement that we should put these helper functions to one place if we do so. But I'm not sure about the package's name.
I just found that tp-qemu has some utilities under the provider, so we may just as well have a try.1)One of potential issue is that it will largely increase risk of Circular Dependencies import if test case can import module in other test case folder
[...]
I think circularity is not a big issue by having the functions in a higher level module (e.g. provider) and making sure that any code on the higher level only imports external (avocado-/vt). For this specific use case I can't think of a reason why a function on the higher level would want to import code from a lower level test script.
CC @Yingshun
@chunfuwen, Sebas and I have reached an agreement that we should put these helper functions to one place if we do so. But I'm not sure about the package's name.
I just found that tp-qemu has some utilities under the provider, so we may just as well have a try.1)One of potential issue is that it will largely increase risk of Circular Dependencies import if test case can import module in other test case folder
[...]I think circularity is not a big issue by having the functions in a higher level module (e.g.
provider) and making sure that any code on the higher level only imports external (avocado-/vt). For this specific use case I can't think of a reason why a function on the higher level would want to import code from a lower level test script.CC @Yingshun
@smitterl
https://github.com/autotest/tp-libvirt/pull/3481/files already experiment usage of provider in tp-libvirt