Re: [review-request] adamian/20150715_testfixes

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

Hi Alex,

Great!
Submitted your patches.

I've noticed that you didn't change sorting as I suggested.
Can you tell what's the reason?


Regards,
Ed

On Thu, Jul 30, 2015 at 12:35:15PM +0100, Damian, Alexandru wrote:
> 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@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@yoctoproject.org
> > > > https://lists.yoctoproject.org/listinfo/toaster
> > >
> > >
> > > --
> > > --
> > > Regards,
> > > Ed
> > > --
> > > _______________________________________________
> > > toaster mailing list
> > > toaster@yoctoproject.org
> > > https://lists.yoctoproject.org/listinfo/toaster
> >
> > --
> > --
> > Regards,
> > Ed
> >
> 
> 
> 
> -- 
> Alex Damian
> Yocto Project
> SSG / OTC

--
Regards,
Ed