From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: by yocto-www.yoctoproject.org (Postfix, from userid 118) id A063EE00A97; Thu, 30 Jul 2015 01:25:49 -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: * -5.0 RCVD_IN_DNSWL_HI RBL: Sender listed at http://www.dnswl.org/, high * trust * [134.134.136.24 listed in list.dnswl.org] * -1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1% * [score: 0.0000] Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by yocto-www.yoctoproject.org (Postfix) with ESMTP id BB6F9E00A7E for ; Thu, 30 Jul 2015 01:25:46 -0700 (PDT) Received: from orsmga003.jf.intel.com ([10.7.209.27]) by orsmga102.jf.intel.com with ESMTP; 30 Jul 2015 01:25:30 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.15,576,1432623600"; d="scan'208";a="615879388" Received: from linux.intel.com ([10.23.219.25]) by orsmga003.jf.intel.com with ESMTP; 30 Jul 2015 01:25:30 -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 1B2796A4005; Thu, 30 Jul 2015 01:24:44 -0700 (PDT) Date: Thu, 30 Jul 2015 11:25:25 +0300 From: Ed Bartosh To: "Damian, Alexandru" Message-ID: <20150730082525.GA27082@linux.intel.com> References: <20150729212711.GA22626@linux.intel.com> MIME-Version: 1.0 In-Reply-To: <20150729212711.GA22626@linux.intel.com> 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 08:25:49 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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