On Tue, Dec 13, 2016 at 06:24:57PM -0600, Jair Gonzalez wrote: > Hi Ed, > > Thank you for your response and suggestions. Below are my comments. >
> > > + @testcase(1557) > > > + def test_listed_images_help(self): > > > + """Test wic listed images help""" > > > + output = runCmd('wic list images').output > > > + imageDetails = [line.split() for line in output.split('\n')] > > > + imageList = [row[0] for row in imageDetails] > > How about replacing two last lines with this? > > imagelist = [line.split()[0] for line in output.split('\n')] > I agree. What about this? > imagelist = [line.split()[0] for line in output.splitlines()] This is event better, thanks! > > > + > > > + "--image-name=core-image-minimal").status) > > Is '=' mandatory here? > On wic's help it appears as mandatory, but on practice, it can be used both > ways. I decided to use both ways along the module to test both usages and > increase coverage, but not to dedicate specific test cases to each > combination. Makes sense to me. > > > def test_compress_gzip(self): > > > """Test compressing an image with gzip""" > > > self.assertEqual(0, runCmd("wic create directdisk " > > > - "--image-name core-image-minimal " > > > + "-e core-image-minimal " > > --image-name is more readable than -e from my point of view. > Similarly to the '=' to define long option names' arguments, I used both > forms of each option along the module to increase coverage. OK > > > + def test_debug_skip_build_check_and_build_rootfs(self): > > > + """Test wic debug, skip-build-check and build_rootfs""" > > > + self.assertEqual(0, runCmd("wic create directdisk " > > > + "--image-name=core-image-minimal " > > > + "-D -s -f").status) > > > + self.assertEqual(1, len(glob(self.resultdir + > "directdisk-*.direct"))) > > > + self.assertEqual(0, runCmd("wic create directdisk " > > > + "--image-name=core-image-minimal " > > > + "--debug " > > > + "--skip-build-check " > > > + "--build-rootfs").status) > > > + self.assertEqual(1, len(glob(self.resultdir + > > > + "directdisk-*.direct"))) > > > + > > I'd split this to two test cases as they're testing two different options. > Actually, those are three different options (with their short and long > versions). I did this to not add too many test cases, but as you mention, > probably it's better to separate them by option to make it clearer. Agreed. > core-image-minimal").status) > > > - self.assertEqual(1, len(glob(self.resultdir + "HYBRID_ISO_IMG- > > *.direct"))) > > > - self.assertEqual(1, len(glob(self.resultdir + > "HYBRID_ISO_IMG-*.iso"))) > > > + "--image-name core-image-minimal" > > > + ).status) > > This is less readable. Is this only to fit the line into 80 chars? > > If so, let's not do it. Lines up to 100 chars long are more readable than > this I > > believe. > I changed it to conform to PEP8 and increase readability on editors adjusted > to 80 chars. However, if you consider it's better to leave it on 100 chars, > I could work within that. Let's agree on max 100 chars/line if it improves readability. Otherwise 80 is ok. > > > + self.assertEqual(0, runCmd("wic create directdisk " > > > + "--image-name=%s " > > > + "--vars %s" > > > + % (image, imgenvdir)).status) > > > + > > Do we really want to test short and long variant of options? > > If so, we should do it for all options. > Within the module, all short and long variant of options are tested. Not all > combinations of long variants with '=' and without it are tested, though. OK, makes sense to me. > > > + @testcase(1562) > > > + def test_alternate_output_dir(self): > > > + """Test alternate output directory""" > > > + self.assertEqual(0, runCmd("wic create directdisk " > > > + "-e core-image-minimal " > > > + "-o %s" > > > + % self.alternate_resultdir).status) > > > + self.assertEqual(1, len(glob(self.alternate_resultdir + > > > + "build/directdisk-*.direct"))) > > > + self.assertEqual(0, runCmd("wic create mkefidisk -e " > > > + "core-image-minimal " > > > + "--outdir %s" > > > + % self.alternate_resultdir).status) > > > + self.assertEqual(1, len(glob(self.alternate_resultdir + > > > + "build/mkefidisk-*direct"))) > > Would one test be enough? > I'm using both output option variants to increase coverage. yep, understood. > > BTW, did you measure how long does the test run before and after your > > changes? We should be careful here as this test runs on ab and makes > people > > wait longer for results. > Agreed. I didn't do it, but I'll measure it and take it into account for my > next update. Great! It would be interesting to look at the numbers. -- Regards, Ed -- _______________________________________________ Openembedded-core mailing list Openembedded-core@lists.openembedded.org http://lists.openembedded.org/mailman/listinfo/openembedded-core