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

[sujith h]

On Mon, Aug 10, 2015 at 9:56 PM, Ed Bartosh <ed.bartosh at linux.intel.com>
wrote:

> 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 at gmail.com>
> > Signed-off-by: Sujith Haridasan <Sujith_Haridasan at 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.
>

Thanks for detailed feed back. I will make modifications accordingly and
re-submit the patch.

Thanks,
Sujith H

-- 
സുജിത് ഹരിദാസന്
Bangalore
<Project>Contributor to KDE project
http://fci.wikia.com/wiki/Anti-DRM-Campaign
<Blog> http://sujithh.info
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.yoctoproject.org/pipermail/toaster/attachments/20150811/abe1cbf8/attachment.html>