Re: [review-request][PATCH][v2] toastergui: Add tests for xhr_importlayer

[Ed Bartosh <ed.bartosh@linux.intel.com>]

Hi Sujith,

Thank you for updating your patch.

My comments are below.

On Mon, Aug 10, 2015 at 02:33:10PM +0530, Sujith H wrote:
> Inorder to make sure if layer name which we import is already
> available in toaster database, a test case has been included.
> This would help user to identify if layer name which has been
> provided in the import layer web page already exists or not.
> 
> Signed-off-by: Sujith Haridasan <sujith.h@gmail.com>
> Signed-off-by: Sujith Haridasan <Sujith_Haridasan@mentor.com>
> ---
>  bitbake/lib/toaster/toastergui/tests.py | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/bitbake/lib/toaster/toastergui/tests.py b/bitbake/lib/toaster/toastergui/tests.py
> index 93a85ac..2986e57 100644
> --- a/bitbake/lib/toaster/toastergui/tests.py
> +++ b/bitbake/lib/toaster/toastergui/tests.py
> @@ -148,3 +148,35 @@ class ViewTests(TestCase):
>              # After "typeing" the alpabet we should have result true
>              # from each of the urls
>              self.assertTrue(results)
> +
> +class XHRImportLayerTestCase(ViewTests):
> +    """ Tests to verify xhr_importlayer API"""
> +    def setUp(self):
> +        super(XHRImportLayerTestCase, self).setUp()
Do you really need a new class for your test case?
If you add it to ViewTests you don't need above 4 lines.

> +
> +    def test_import(self):
> +        """Tests to verify import layer already exist, if the imported layer is ok, response
> +        for empty data, response for html tag in data"""
Please, split dosctring to subject line and the body to make output of
./manage.py test -v2 toastergui meaningful.

> +        #Test for importing an already existing layer
> +        data = {'vcs_url' : "git://git.example.com/test",
I'd suggest to rename data->args.
> +                'name' : "base-layer", 'git_ref': "c12b9596afd236116b25ce26dbe0d793de9dc7ce",
> +                'project_id': 1, 'dir_path' : "/path/in/repository"}
> +        result = self.client.post(reverse('xhr_importlayer'), data)
> +        self.assertEqual(result.status_code, 200)
> +        self.assertTrue("hint-layer-exists-with-different-url" in result.content)
It would be better to convert result from json using json.load as it's
done in other test cases, i.e.
data = json.loads(response.content)
self.assertNotEqual(data["error"], "ok")

> +        #Test to verify import of a layer successful
> +        data['name'] = "meta-oe"
> +        result = self.client.post(reverse('xhr_importlayer'), data)
> +        self.assertTrue('"error": "ok"' in result.content)
> +
> +        #Test for html tag in the data
> +        data['<'] = "testing html tag"
> +        result = self.client.post(reverse('xhr_importlayer'), data)
> +        self.assertTrue('"error": "Invalid character <"' in result.content)
> +
> +        #Empty data passed
> +        data = {}
> +        result = self.client.post(reverse('xhr_importlayer'), data)
> +        self.assertTrue('"error": "Missing parameters; requires vcs_url, \
> +                        name, git_ref and project_id"' in result.content)
Would it be enough to check that error is not 'ok'? If you change a
message you will not have to change test code.

Please, make your code similar to the existing code. Use the same
vairable names, checks, etc. It would make test code look and behave in
a consistent way.

--
Regards,
Ed