[Toaster] [review-request] adamian/20150715_testfixes

[Damian, Alexandru]

Hello,

I have pushed a new version for review, on the same branch:

adamian/20150715_testfixes

It tackles all the points you raised, and also brings in a new patch that
tackles pylint issues specifically.

I am running pylint with some tests disabled:
disable=logging-too-many-args,line-too-long,missing-docstring

And I get 10.00/10 code rating.

Can you please review and submit ?

Cheers,
Alex


On Thu, Jul 30, 2015 at 9:25 AM, Ed Bartosh <ed.bartosh at linux.intel.com>
wrote:

> Hi Alex,
>
> As you're in tts it's good time to pylint it.
> The code is relatively small, so it shouldn't take much time.
>
> JFYI, here are some pylint statistics for tts code:
> $ pylint --report n *.py |wc -l
> 455
>
> $ pylint *.py |grep rated
> Your code has been rated at 2.49/10
>
> Regards,
> Ed
>
> On Thu, Jul 30, 2015 at 12:27:11AM +0300, Ed Bartosh wrote:
> > Hi Alex,
> >
> > Your patchset looks ok for me.
> >
> > Just a bit of nitpicking over code quality:
> >
> > 1. Your change in shellutils.py caused new pylint error:
> > E: 91, 0: inconsistent use of tabs and spaces in indentation
> > (syntax-error)
> >
> > 2.
> > -        if len(result.errors) > 0:
> > -            map(lambda x: config.logger.error("Exception on test: %s" %
> >              pprint.pformat(x)), result.errors)
> > +        for x in result.errors:
> > +            config.logger.error("Exception on test: %s:\n%s\n" %
> > (pprint.pformat(x[0]),
> > +                "\n".join(["--  %s" % x for x in x[1].split("\n")])))
> >
> > I'd suggest not to use format arguments for logging. You can read the
> > detailed explanations here:
> http://docs.pylint.org/features.html#messages
> > Here is pylint warning caused by this:
> > W: 49,23: Specify string format arguments as logging function parameters
> > (logging-not-lazy)
> >
> > There are a lot of this kind of code in your patchset and in the tts
> > code, so fixing all of them in separate commit would be good.
> >
> > 3. one character long variable names(x, e in your changes) are not very
> readable:
> > C: 40, 4: Invalid variable name "e" (invalid-name)
> >
> > 4. it would be better to assign os.path.join(self.crtdir,
> "toaster.sqlite") to local
> > variable:
> > +        if os.path.exists(os.path.join(self.crtdir,
> > "toaster.sqlite")):
> > +            os.remove(os.path.join(self.crtdir, "toaster.sqlite"))
> >
> > 5. Mixed 2 different set of changes in commit " fix HTML5 compliance
> > test". It's better to move logging improvements to another commit.
> >
> > 6. Typo in "execute tests in numeric order" commit message:
> > undeeded -> unneeded
> >
> > 7. As your tests have the same prefix default sorting should be enough,
> > e.g.
> > In [1]: sorted(["Test01PySystemStart", "Test00PyCompilable",
> "Test02HTML5Compliance"])
> > Out[1]: ['Test00PyCompilable', 'Test01PySystemStart',
> 'Test02HTML5Compliance']
> > +        # sorting the tests by the numeric order in the class name
> > +        tests = sorted(tests, key = lambda x: int(re.search(r"[0-9]+",
> > x[1]).group(0)))
> >
> > 8. Mixed functionality and code readability fixes in "run pylint on the
> toaster files"
> >
> > Regards,
> > Ed
> >
> > On Thu, Jul 16, 2015 at 03:44:05PM +0100, Damian, Alexandru wrote:
> > > Hello,
> > >
> > > I've improved the TTS tests a bit. At this moment, the tests spot YOCTO
> > > #7955 (Toaster fails to retrieve recipe and machine information from
> the
> > > layer index) correctly.
> > >
> > > The patchset is at:
> > >
> > >
> http://git.yoctoproject.org/cgit/cgit.cgi/poky-contrib/log/?h=adamian/20150715_testfixes
> > >
> >
> > >
> > > The toaster unit tests and the selenium tests are not run - it's going
> to
> > > be a different patchset on top of this one.
> > >
> > > Please review and merge at your own convenience.
> > >
> > > Cheers,
> > > Alex
> > >
> > >
> > > --
> > > Alex Damian
> > > Yocto Project
> > > SSG / OTC
> >
> > > --
> > > _______________________________________________
> > > toaster mailing list
> > > toaster at yoctoproject.org
> > > https://lists.yoctoproject.org/listinfo/toaster
> >
> >
> > --
> > --
> > Regards,
> > Ed
> > --
> > _______________________________________________
> > toaster mailing list
> > toaster at yoctoproject.org
> > https://lists.yoctoproject.org/listinfo/toaster
>
> --
> --
> Regards,
> Ed
>



-- 
Alex Damian
Yocto Project
SSG / OTC
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.yoctoproject.org/pipermail/toaster/attachments/20150730/3e255149/attachment-0001.html>