Test CA226886: test import.vdi when SR is unavailable for any host.

[minli1]

This is a unit test for CA226886 code change. Test import_raw_vdi.import when the sr is unavailable for any host.It should raise exn and set the task to fail.
I factor the existing code to make it more suitable for unit testing. Since it's easier than writing socket server end /client end codes to create a real socket in unit-test.

[minli1]

Hi, @gaborigloi and @robhoes
Could you please help to review?
Since I had factored the existing code in import_raw_vdi.ml to make it more suitable for unit testing.
But when I writing a unit test, it enters fail_task_in_request to make the task failed, it's also needed a socket which I can't bypass. So I had to assert_raises for Unix.Unix_error(Unix.ENOTCONN, "write", "").
I am wondering if there is a better way in Ounit to compare the results.
Any comments will be great and thankful.

[coveralls]

Coverage increased (+0.1%) to 14.864% when pulling 471b494 on minli1:private/minli/CA-226886 into ac7e222 on xapi-project:master.

[jonludlam]

I'm not sure I quite understand this. You're changing the external behaviour of one of the HTTP handlers - why are you dong that and what is the effect? That change needs to be in a separate commit from any test, so we also need to split this PR into at least 2 commits.

[minli1]

Hi, Jon. @jonludlam Thanks for you reply.

1. I change this import function by removing the Xapi_http.assert_credentials_ok to the handler. This is because I don't put a real connected socket into the params, the Xapi_http.assert_credentials_ok will fail if the socket is not connected.
2. So I factor the existing code just like this test which was told by @robhoes.
   0c62b77.
3. It's not quite a better way to write socket server/client code to create a real socket, right? Since unit test does not a place for testing inter-process communication.
4. Yes,I can spilt this PR into 2 commits.

Thanks a lot.
BR.

[gaborigloi]

Hmm, this function seems to be quite hard to unit test sadly :/
To test the new behaviour, we should test

1. That the task passed in the query is set to failed

2. That an HTTP error is returned to the client

I think we can only test the first thing here, maybe we could set up the socket somehow to receive the HTTP response, but that might be too complicated for a unit test.

[minli1]

Thanks, @gaborigloi.
You mean we still need somehow set up the socket to receive the HTTP response, right?
But it seems to need to set up socket server end and client end. We may create it in a more common place and use it, so next time someone else will use them.
Please correct if I misunderstanding.

[gaborigloi]

Hi @minli1 , I think setting up the socket might be impossible or very complicated :(. So I think we can maybe only unit test item 1), that the task is set to failed. Sorry, my comment was misleading when I said that we could a unit test for this - I expected it to be much easier to unit test this function. Maybe we'll just have to write a XenRT test in the end, if we cannot unit test 1) either.

[gaborigloi]

So I think what we could still try is to pass a task in the req parameter, and ignore any exceptions that happen (including Unix.Unix_error(Unix.ENOTCONN, "write", "")), and observer the status of that task when the function returned, it should be "failed".

[minli1]

Thanks @gaborigloi
I will update the PR then.

[minli1]

Hi, @gaborigloi
I finally passed the task in the request and using Ounit.assert_equal to verify the task is set failed when the function returned. It worked in unit test.
For this, I also refactor some existing code which suggested by @robhoes to make them more suitable for unit test.
I split the commit into two, one for refactor and one for unit test only.
Please help to review, thanks in advance.

[coveralls]

Coverage increased (+0.01%) to 14.992% when pulling 1d57894 on minli1:private/minli/CA-226886 into 3eafb5e on xapi-project:master.

[coveralls]

Coverage increased (+0.4%) to 15.376% when pulling 1d57894 on minli1:private/minli/CA-226886 into 3eafb5e on xapi-project:master.

[jonludlam]

I wonder if it would make more sense to test this from quicktest?

[gaborigloi]

Agreed, this seems to be hard to unit test sadly, it's so connected to other things.

[minli1]

Hi, @gaborigloi
I am wondering is this possible to test from quicktest? I will have a look. It may also need to refactor even in quicktest.
Thanks.

[gaborigloi]

@minli1 Run /opt/xensource/debug/quicktest on a XenServer host.

[minli1]

HI, Jon and Gabor. @jonludlam @gaborigloi
I raised a new PR for a quicktest for this. See: #3152
Please help to review. Thanks!

[stephen-turner]

Does the quicktest PR replace this one? If so, let's close this one. Thank you.

[mseri]

Looks like a duplicate. Closing for now. Please reopen if I am wrong