[libvirt] [PATCH v2 07/10] viriscsitest: Test virISCSIConnectionLogin

[John Ferlan]

On 07/24/2018 04:25 AM, Michal Privoznik wrote:
> On 07/23/2018 03:55 PM, John Ferlan wrote:
>>
>>
>> On 07/23/2018 08:34 AM, Michal Prívozník wrote:
>>> On 07/23/2018 02:12 PM, John Ferlan wrote:
>>>>
>>>>
>>>> On 07/23/2018 04:01 AM, Michal Prívozník wrote:
>>>>> On 07/17/2018 09:14 PM, John Ferlan wrote:
>>>>>>
>>>>>>
>>>>>> On 07/04/2018 05:23 AM, Michal Privoznik wrote:
>>>>>>> Introduce one basic test that tests the simplest case:
>>>>>>> logging into portal without any IQN.
>>>>>>>
>>>>>>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>>>>>>> ---
>>>>>>>  tests/viriscsitest.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>  1 file changed, 46 insertions(+)
>>>>>>>
>>>>>>> diff --git a/tests/viriscsitest.c b/tests/viriscsitest.c
>>>>>>> index 3bb3993196..a7287069ba 100644
>>>>>>> --- a/tests/viriscsitest.c
>>>>>>> +++ b/tests/viriscsitest.c
>>>>>>> @@ -94,6 +94,16 @@ static void testIscsiadmCb(const char *const*args,
>>>>>>>                 args[8] && STREQ(args[8], "nonpersistent") &&
>>>>>>>                 args[9] == NULL) {
>>>>>>>          ignore_value(VIR_STRDUP(*output, iscsiadmSendtargetsOutput));
>>>>
>>>> here we do some sort of comparison
>>>
>>> What comparison? The only comparison I see is "args[X] && STREQ(args[X],
>>> ...)".
>>>
>>> If you're referring to VIR_STRDUP() that is setting the pretended output
>>> of the iscsiadm command. It's not comparing anything.
>>>
>>
>> Can you just indicate rather than "nada" what the actual deal is here?
> 
> Okay, how about "/* no output */" instead of "nada"?
> 

Not convinced that's better than nada? Mainly because it's not true and
the example below I believe proves that point. As poor of an API to
configure things as iscsiadm is, it can be quite chatty for output.

Replace -T with --targetname and -p with --portal. Not only is there
visual output, but (at least on my host) there's a sequence of clicks
that happens on login and logout.

>> That is, in our mocked environment when running this command we don't
>> expect to get any output from iscsiadm. If this were a real command then
>> the following would be returned:
>>> ...
>>
>> In my saved iSCSI output file, I have for example:
>>
>> iscsiadm -m node -T iqn.2013-12.com.example:iscsi-chap-pool -p
>> 192.168.122.1 --login
>>
>> returning:
>>
>> Logging in to [iface: default, target:
>> iqn.2013-12.com.example:iscsi-chap-pool, portal: 192.168.122.1,3260]
>> (multiple)
>> Login to [iface: default, target:
>> iqn.2013-12.com.example:iscsi-chap-pool, portal: 192.168.122.1,3260]
>> successful.
> 
> But this output is never processed by our code, therefore it doesn't
> make much sense for our 'mock' to produce any.
> 

Not exactly the point compared to the 3 strings already present in the
code that are in a way processed, but I do understand what you're stating.

Remove yourself from the authorship of the code and you don't find it
strange that some commands aren't producing mock output?  Reviewing such
code would you just be satisfied with "/* nada */"? Wouldn't you want to
read the code, try an example or two, and then make a decision how to
proceed. If during your examples you found output, then where do you
stand on "nada" or "no output"?

I really don't think it's that much of an ask or a reach to supply
information that would instantaneously help someone looking at this code
to know what is going on.  Reading "nada" or "no output" doesn't help.

Reading something like:

 /* mocking real environment output is not feasible for [creating |
updating | logging into], example of real environment is:

xxx

 */

I believe is better - it's not difficult to add, but I'm at the point of
not caring right now because this truly has gone on too long.

For the code/logic:

Reviewed-by: John Ferlan <jferlan at redhat.com>

Do whatever you want for the comments. I disagree that "nada" or "no
output" is good enough, but it's not worth holding this up.

John