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 <[email protected]> > 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 > > > > [email protected] > > > > https://lists.yoctoproject.org/listinfo/toaster > > > > > > > > > -- > > > -- > > > Regards, > > > Ed > > > -- > > > _______________________________________________ > > > toaster mailing list > > > [email protected] > > > https://lists.yoctoproject.org/listinfo/toaster > > > > -- > > -- > > Regards, > > Ed > > > > > > -- > Alex Damian > Yocto Project > SSG / OTC -- Regards, Ed -- _______________________________________________ toaster mailing list [email protected] https://lists.yoctoproject.org/listinfo/toaster
