| Message ID | 20170929022713.2967-2-ricardo.martincoski@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/5] support/testing: standardize defconfig fragments style | expand |
Ricardo, All, On 2017-09-28 23:27 -0300, Ricardo Martincoski spake thusly: > Postpone the strip out of leading spaces in defconfig fragments from the > __init__ to the setUp method. It allows test cases to post-process the > defconfig in their own __init__ before calling the __init__ method from > the base class. Ideally, this should have been the very first patch in the series. > Indent the only fragment in the tree that currently need this (in the > ccache test case), taking advantage of > "cf3cd4388a support/tests: allow properly indented config fragment". And then that wcould have gone in the other patch without any issue. Once you split and reorder the patches, you can add my: Acked-by: "Yann E. MORIN" <yann.morin.1998@free.fr> Regards, Yann E. MORIN. > Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com> > --- > Tested by hacking support/testing/infra/builder.py > @@ -30,2 +30,3 @@ class Builder(object): > raise SystemError("Cannot olddefconfig") > + raise SystemError("Stop") > and diffing the resulting *-build.log files against old ones: few empty > lines are added/removed. Also tested diffing the resulting .config files > against old ones: they are the same. > > Warnings from flake8 change from 100 to 99. > --- > support/testing/infra/basetest.py | 4 ++-- > support/testing/tests/toolchain/test_external.py | 8 ++++---- > 2 files changed, 6 insertions(+), 6 deletions(-) > > diff --git a/support/testing/infra/basetest.py b/support/testing/infra/basetest.py > index d205119b2c..c41852949a 100644 > --- a/support/testing/infra/basetest.py > +++ b/support/testing/infra/basetest.py > @@ -42,8 +42,6 @@ class BRTest(unittest.TestCase): > self.testname = self.__class__.__name__ > self.builddir = self.outputdir and os.path.join(self.outputdir, self.testname) > self.emulator = None > - self.config = '\n'.join([line.lstrip() for line in > - self.config.splitlines()]) + '\n' > self.config += "BR2_JLEVEL={}\n".format(self.jlevel) > > def show_msg(self, msg): > @@ -51,6 +49,8 @@ class BRTest(unittest.TestCase): > self.testname, msg) > def setUp(self): > self.show_msg("Starting") > + self.config = '\n'.join([line.lstrip() for line in > + self.config.splitlines()]) + '\n' > self.b = Builder(self.config, self.builddir, self.logtofile) > > if not self.keepbuilds: > diff --git a/support/testing/tests/toolchain/test_external.py b/support/testing/tests/toolchain/test_external.py > index 1bb5e9497b..ad2f56a20e 100644 > --- a/support/testing/tests/toolchain/test_external.py > +++ b/support/testing/tests/toolchain/test_external.py > @@ -232,10 +232,10 @@ class TestExternalToolchainBuildrootuClibc(TestExternalToolchain): > > class TestExternalToolchainCCache(TestExternalToolchainBuildrootuClibc): > extraconfig = \ > -""" > -BR2_CCACHE=y > -BR2_CCACHE_DIR="{builddir}/ccache-dir" > -""" > + """ > + BR2_CCACHE=y > + BR2_CCACHE_DIR="{builddir}/ccache-dir" > + """ > > def __init__(self, names): > super(TestExternalToolchainBuildrootuClibc, self).__init__(names) > -- > 2.13.0 >
Hello, Thank you for the review of the series. I will reply to this one and also to patch 4. On Fri, Sep 29, 2017 at 05:17 AM, Yann E. MORIN wrote: > On 2017-09-28 23:27 -0300, Ricardo Martincoski spake thusly: >> Postpone the strip out of leading spaces in defconfig fragments from the >> __init__ to the setUp method. It allows test cases to post-process the >> defconfig in their own __init__ before calling the __init__ method from >> the base class. > > Ideally, this should have been the very first patch in the series. Good idea. I will do, but... > >> Indent the only fragment in the tree that currently need this (in the >> ccache test case), taking advantage of >> "cf3cd4388a support/tests: allow properly indented config fragment". > > And then that wcould have gone in the other patch without any issue. It should. But the reordering shows the jlevel handling is fragile. Both the rootfs overlay and yaffs2 tests currently have defconfig fragments that don't end in a newline, so the new patch 1 ends up silently disabling jlevel: ...s/core/rootfs-overlay2"BR2_JLEVEL=1 BR2_TARGET_ROOTFS_YAFFS2=yBR2_JLEVEL=1 So I plan to add another patch before the series, to make jlevel handling more robust (to other changes in the infra) and at same time use the same style. self.config += \ """ BR2_JLEVEL={} """.format(self.jlevel) It also made me realize that this patch implies that I need to resend another one (tests for the git downloader) since there I override the setUp method. http://patchwork.ozlabs.org/patch/806161/ I would need to duplicate there the logic to remove the indentation of defconfig fragments. Perhaps instead of moving the logic to the setUp of BRTest I could move it to the builder init. This way any new test class that reimplements setUp and uses builder will "inherit" it. Those that reimplement setUp but don't use builder don't need that code. class Builder(object): def __init__(self, config, builddir, logtofile): self.config = '\n'.join([line.lstrip() for line in config.splitlines()]) + '\n' What do you think? > > Once you split and reorder the patches, you can add my: > > Acked-by: "Yann E. MORIN" <yann.morin.1998@free.fr> > [snip] >> +++ b/support/testing/infra/basetest.py >> @@ -42,8 +42,6 @@ class BRTest(unittest.TestCase): >> self.testname = self.__class__.__name__ >> self.builddir = self.outputdir and os.path.join(self.outputdir, self.testname) >> self.emulator = None >> - self.config = '\n'.join([line.lstrip() for line in >> - self.config.splitlines()]) + '\n' >> self.config += "BR2_JLEVEL={}\n".format(self.jlevel) >> >> def show_msg(self, msg): >> @@ -51,6 +49,8 @@ class BRTest(unittest.TestCase): >> self.testname, msg) >> def setUp(self): >> self.show_msg("Starting") >> + self.config = '\n'.join([line.lstrip() for line in >> + self.config.splitlines()]) + '\n' >> self.b = Builder(self.config, self.builddir, self.logtofile) >> >> if not self.keepbuilds: [snip] Regards, Ricardo
Ricardo, All, On 2017-10-01 22:03 -0300, Ricardo Martincoski spake thusly: > On Fri, Sep 29, 2017 at 05:17 AM, Yann E. MORIN wrote: > > On 2017-09-28 23:27 -0300, Ricardo Martincoski spake thusly: > >> Postpone the strip out of leading spaces in defconfig fragments from the > >> __init__ to the setUp method. It allows test cases to post-process the > >> defconfig in their own __init__ before calling the __init__ method from > >> the base class. > > > > Ideally, this should have been the very first patch in the series. > > Good idea. I will do, but... > > > > >> Indent the only fragment in the tree that currently need this (in the > >> ccache test case), taking advantage of > >> "cf3cd4388a support/tests: allow properly indented config fragment". > > > > And then that wcould have gone in the other patch without any issue. > > It should. But the reordering shows the jlevel handling is fragile. > > Both the rootfs overlay and yaffs2 tests currently have defconfig fragments that > don't end in a newline, so the new patch 1 ends up silently disabling jlevel: > ...s/core/rootfs-overlay2"BR2_JLEVEL=1 > BR2_TARGET_ROOTFS_YAFFS2=yBR2_JLEVEL=1 Weird, because on line 46, we explicitly add a trainling '\n' : https://git.buildroot.org/buildroot/tree/support/testing/infra/basetest.py#n45 45: self.config = '\n'.join([line.lstrip() for line in self.config.splitlines()]) + '\n' 46: self.config += "BR2_JLEVEL={}\n".format(self.jlevel) > So I plan to add another patch before the series, to make jlevel handling more > robust (to other changes in the infra) and at same time use the same style. > self.config += \ > """ > BR2_JLEVEL={} > """.format(self.jlevel) Well, I doubt this is useful. Just prepend a \n, if it really is useful : self.config += "\nBR2_JLEVEL={}\n".format(self.jlevel) > It also made me realize that this patch implies that I need to resend another > one (tests for the git downloader) since there I override the setUp method. > http://patchwork.ozlabs.org/patch/806161/ > I would need to duplicate there the logic to remove the indentation of defconfig > fragments. > > Perhaps instead of moving the logic to the setUp of BRTest I could move it to > the builder init. This way any new test class that reimplements setUp and uses > builder will "inherit" it. Those that reimplement setUp but don't use builder > don't need that code. > class Builder(object): > def __init__(self, config, builddir, logtofile): > self.config = '\n'.join([line.lstrip() for line in > config.splitlines()]) + '\n' > What do you think? Yes, that would probably be better. Regards, Yann E. MORIN.
Hello, On Mon, Oct 02, 2017 at 02:49 AM, Yann E. MORIN wrote: > On 2017-10-01 22:03 -0300, Ricardo Martincoski spake thusly: >> On Fri, Sep 29, 2017 at 05:17 AM, Yann E. MORIN wrote: >> > On 2017-09-28 23:27 -0300, Ricardo Martincoski spake thusly: >> >> Postpone the strip out of leading spaces in defconfig fragments from the >> >> __init__ to the setUp method. It allows test cases to post-process the >> >> defconfig in their own __init__ before calling the __init__ method from >> >> the base class. >> > >> > Ideally, this should have been the very first patch in the series. >> >> Good idea. I will do, but... >> >> > >> >> Indent the only fragment in the tree that currently need this (in the >> >> ccache test case), taking advantage of >> >> "cf3cd4388a support/tests: allow properly indented config fragment". >> > >> > And then that wcould have gone in the other patch without any issue. >> >> It should. But the reordering shows the jlevel handling is fragile. Fragile to other changes in the infra, I meant. >> >> Both the rootfs overlay and yaffs2 tests currently have defconfig fragments that >> don't end in a newline, so the new patch 1 ends up silently disabling jlevel: >> ...s/core/rootfs-overlay2"BR2_JLEVEL=1 >> BR2_TARGET_ROOTFS_YAFFS2=yBR2_JLEVEL=1 > > Weird, because on line 46, we explicitly add a trainling '\n' : > https://git.buildroot.org/buildroot/tree/support/testing/infra/basetest.py#n45 > > 45: self.config = '\n'.join([line.lstrip() for line in > self.config.splitlines()]) + '\n' > 46: self.config += "BR2_JLEVEL={}\n".format(self.jlevel) But line 45 is the line being moved by this patch. So line 46 becomes "unprotected" from defconfig fragments that does not end in a newline. > >> So I plan to add another patch before the series, to make jlevel handling more >> robust (to other changes in the infra) and at same time use the same style. >> self.config += \ >> """ >> BR2_JLEVEL={} >> """.format(self.jlevel) > > Well, I doubt this is useful. Just prepend a \n, if it really is useful : > > self.config += "\nBR2_JLEVEL={}\n".format(self.jlevel) OK. > >> It also made me realize that this patch implies that I need to resend another >> one (tests for the git downloader) since there I override the setUp method. >> http://patchwork.ozlabs.org/patch/806161/ >> I would need to duplicate there the logic to remove the indentation of defconfig >> fragments. >> >> Perhaps instead of moving the logic to the setUp of BRTest I could move it to >> the builder init. This way any new test class that reimplements setUp and uses >> builder will "inherit" it. Those that reimplement setUp but don't use builder >> don't need that code. >> class Builder(object): >> def __init__(self, config, builddir, logtofile): >> self.config = '\n'.join([line.lstrip() for line in >> config.splitlines()]) + '\n' >> What do you think? > > Yes, that would probably be better. I will do this. Regards, Ricardo
diff --git a/support/testing/infra/basetest.py b/support/testing/infra/basetest.py index d205119b2c..c41852949a 100644 --- a/support/testing/infra/basetest.py +++ b/support/testing/infra/basetest.py @@ -42,8 +42,6 @@ class BRTest(unittest.TestCase): self.testname = self.__class__.__name__ self.builddir = self.outputdir and os.path.join(self.outputdir, self.testname) self.emulator = None - self.config = '\n'.join([line.lstrip() for line in - self.config.splitlines()]) + '\n' self.config += "BR2_JLEVEL={}\n".format(self.jlevel) def show_msg(self, msg): @@ -51,6 +49,8 @@ class BRTest(unittest.TestCase): self.testname, msg) def setUp(self): self.show_msg("Starting") + self.config = '\n'.join([line.lstrip() for line in + self.config.splitlines()]) + '\n' self.b = Builder(self.config, self.builddir, self.logtofile) if not self.keepbuilds: diff --git a/support/testing/tests/toolchain/test_external.py b/support/testing/tests/toolchain/test_external.py index 1bb5e9497b..ad2f56a20e 100644 --- a/support/testing/tests/toolchain/test_external.py +++ b/support/testing/tests/toolchain/test_external.py @@ -232,10 +232,10 @@ class TestExternalToolchainBuildrootuClibc(TestExternalToolchain): class TestExternalToolchainCCache(TestExternalToolchainBuildrootuClibc): extraconfig = \ -""" -BR2_CCACHE=y -BR2_CCACHE_DIR="{builddir}/ccache-dir" -""" + """ + BR2_CCACHE=y + BR2_CCACHE_DIR="{builddir}/ccache-dir" + """ def __init__(self, names): super(TestExternalToolchainBuildrootuClibc, self).__init__(names)
Postpone the strip out of leading spaces in defconfig fragments from the __init__ to the setUp method. It allows test cases to post-process the defconfig in their own __init__ before calling the __init__ method from the base class. Indent the only fragment in the tree that currently need this (in the ccache test case), taking advantage of "cf3cd4388a support/tests: allow properly indented config fragment". Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com> --- Tested by hacking support/testing/infra/builder.py @@ -30,2 +30,3 @@ class Builder(object): raise SystemError("Cannot olddefconfig") + raise SystemError("Stop") and diffing the resulting *-build.log files against old ones: few empty lines are added/removed. Also tested diffing the resulting .config files against old ones: they are the same. Warnings from flake8 change from 100 to 99. --- support/testing/infra/basetest.py | 4 ++-- support/testing/tests/toolchain/test_external.py | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-)