From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: by yocto-www.yoctoproject.org (Postfix, from userid 118) id 8294CE00AE9; Thu, 30 Jul 2015 09:29:53 -0700 (PDT) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on yocto-www.yoctoproject.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 X-Spam-HAM-Report: * -1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1% * [score: 0.0000] * -5.0 RCVD_IN_DNSWL_HI RBL: Sender listed at http://www.dnswl.org/, high * trust * [192.55.52.115 listed in list.dnswl.org] Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by yocto-www.yoctoproject.org (Postfix) with ESMTP id 2F259E00AAA for ; Thu, 30 Jul 2015 09:29:48 -0700 (PDT) Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga103.fm.intel.com with ESMTP; 30 Jul 2015 09:29:48 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.15,577,1432623600"; d="scan'208";a="616144935" Received: from linux.intel.com ([10.23.219.25]) by orsmga003.jf.intel.com with ESMTP; 30 Jul 2015 09:29:48 -0700 Received: from linux.intel.com (vmed.fi.intel.com [10.237.72.65]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by linux.intel.com (Postfix) with ESMTP id E5E926A4005; Thu, 30 Jul 2015 09:29:02 -0700 (PDT) Date: Thu, 30 Jul 2015 19:29:43 +0300 From: Ed Bartosh To: "Damian, Alexandru" Message-ID: <20150730162943.GA8013@linux.intel.com> References: <20150729212711.GA22626@linux.intel.com> <20150730082525.GA27082@linux.intel.com> MIME-Version: 1.0 In-Reply-To: Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo User-Agent: Mutt/1.5.21 (2010-09-15) Cc: "toaster@yoctoproject.org" Subject: Re: [review-request] adamian/20150715_testfixes X-BeenThere: toaster@yoctoproject.org X-Mailman-Version: 2.1.13 Precedence: list Reply-To: ed.bartosh@linux.intel.com List-Id: Web based interface for BitBake List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 30 Jul 2015 16:29:53 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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 > 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