Re: [review-request] adamian/20150715_testfixes

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

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