On 9/27/19 5:32 PM, Daniel Henrique Barboza wrote:
On 9/27/19 5:33 PM, Cole Robinson wrote:
On 9/26/19 10:56 AM, Daniel Henrique Barboza wrote:
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> ---
I've made this test file to make sure I wasn't messing up with the logic at patch 8. The idea of having this test seems okay, but probably I could do it shorter/cleaner.
Feel free to discard it if it's not applicable or unnecessary. Adding this new file make the whole patch series, aimed at reduce code repetition and lines of code, add more lines than before. The irony is real.
It will reduce lines if you fold the data.entityName = X bit into the TEST_ calls, passing the string value as the first argument for example
Got it.
We don't have a ton of fine grained unittesting like this in libvirt, but it doesn't hurt. Though in this case it seems kinda overkill IMO to call it for possible combo that it's used in. I think it's better to just call it in all the invocations to give full code coverage. If we want to test the individual driver usage of the function then we would want to find a way to test those entry points directly IMO. Maybe others have opinions.
Do you mean we should just test the actual usage of the function in the code instead of testing all possible fail scenarios? For example, the code does not call virConnectValidateURIPath("/system", X , false) anywhere, so it's ok to not test them since it's not being exercised anyway - otherwise we're entering TDD territory (which I don't mind - kind of a fan TBH), testing all possible stuff just for sake of testing.
Is that what you're saying? I'm fine with it, and it will cut a good chunk of lines in this file too.
I was thinking, add test cases that are needed to hit every bit of logic in the function. So privileged=false, /session path privileged=false, non-/session path failure privileged=true, /system path privileged=true, non-/system path failure privileged=true, vbox /session privileged=true, qemu /session privileged=true, other driver /session failure - Cole