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

Damian, Alexandru alexandru.damian at intel.com
Thu Jul 30 13:57:25 PDT 2015 

Yes, sorry, I didn't mention that.

The sorting has sense when you can stop testing because you know that if a
test fails, the following tests will fail for sure.
But not all tests have this property - it is possible to have some tests
fail and some tests sane. In fact, the sorting
has sense only for the start-up tests - after you verify that toaster
starts, you can run all other tests.

So I didn't touch the sorting because it's going to be changed soon in the
sense of running sorted tests first, and tests without sorting afterward.

Hope this helps,
Alex

On Thu, Jul 30, 2015 at 5:29 PM, Ed Bartosh <ed.bartosh at linux.intel.com>
wrote:

> 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 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
>
> --
> Regards,
> Ed
>



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

More information about the toaster mailing list