| Message ID | 20170715224949.25598-1-yann.morin.1998@free.fr |
|---|---|
| State | Changes Requested |
| Headers | show |
Hello, On Sun, 16 Jul 2017 00:49:49 +0200, Yann E. MORIN wrote: > Currently, defining a config fragment in the runtime test infra requires > that the fragment not to be indented. This is beark, and causes grievance > when looking at the code (e.g. to fix it). > > Just strip out all leading spaces/tabs when writing the configuration > lines into the config file. Seems good in principle, but could you give an example of an indented config fragment you intend to use? Thanks! Thomas
Thomas, All, On 2017-07-16 09:18 +0200, Thomas Petazzoni spake thusly: > On Sun, 16 Jul 2017 00:49:49 +0200, Yann E. MORIN wrote: > > Currently, defining a config fragment in the runtime test infra requires > > that the fragment not to be indented. This is beark, and causes grievance > > when looking at the code (e.g. to fix it). > > > > Just strip out all leading spaces/tabs when writing the configuration > > lines into the config file. > > Seems good in principle, but could you give an example of an indented > config fragment you intend to use? Sure. Here's a diff against master, for example: diff --git a/support/testing/tests/fs/test_squashfs.py b/support/testing/tests/fs/test_squashfs.py index b205b6a55a..9fad28f834 100644 --- a/support/testing/tests/fs/test_squashfs.py +++ b/support/testing/tests/fs/test_squashfs.py @@ -5,12 +5,12 @@ import infra.basetest class TestSquashfs(infra.basetest.BRTest): config = infra.basetest.BASIC_TOOLCHAIN_CONFIG + \ -""" -BR2_TARGET_ROOTFS_SQUASHFS=y -# BR2_TARGET_ROOTFS_SQUASHFS4_GZIP is not set -BR2_TARGET_ROOTFS_SQUASHFS4_LZ4=y -# BR2_TARGET_ROOTFS_TAR is not set -""" + """ + BR2_TARGET_ROOTFS_SQUASHFS=y + # BR2_TARGET_ROOTFS_SQUASHFS4_GZIP is not set + BR2_TARGET_ROOTFS_SQUASHFS4_LZ4=y + # BR2_TARGET_ROOTFS_TAR is not set + """ def test_run(self): unsquashfs_cmd = ["host/bin/unsquashfs", "-s", "images/rootfs.squashfs"] And here is a big file which is easier to read with indented config fragments: https://git.buildroot.org/~ymorin/git/buildroot/tree/support/testing/tests/init/test_systemd.py?h=yem/systemd-skeleton-2 Regards, Yann E. MORIN.
On 16-07-17 00:49, Yann E. MORIN wrote: > Currently, defining a config fragment in the runtime test infra requires > that the fragment not to be indented. This is beark, and causes grievance > when looking at the code (e.g. to fix it). > > Just strip out all leading spaces/tabs when writing the configuration > lines into the config file. > > Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr> > Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> > --- > support/testing/infra/builder.py | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/support/testing/infra/builder.py b/support/testing/infra/builder.py > index a475bb0a30..81735dec96 100644 > --- a/support/testing/infra/builder.py > +++ b/support/testing/infra/builder.py > @@ -16,7 +16,8 @@ class Builder(object): > > config_file = os.path.join(self.builddir, ".config") > with open(config_file, "w+") as cf: > - cf.write(self.config) > + for line in self.config.splitlines(): > + cf.write("{}\n".format(line.lstrip())) I like nitpicking :-) I would do this in BRTest instead of down here, so that self.config contains a correct config file. In fact, I would do it in its constructor instead of in setUp(), so that it is done only once even if a class has two test functions. I would also do it as a oneliner: self.config = '\n'.join(map(str.lstrip, self.config.splitlines())) but some people don't like oneliners :-) Regards, Arnout > > cmd = ["make", > "O={}".format(self.builddir), >
Arnout, All, On 2017-07-18 21:37 +0200, Arnout Vandecappelle spake thusly: > On 16-07-17 00:49, Yann E. MORIN wrote: > > Currently, defining a config fragment in the runtime test infra requires > > that the fragment not to be indented. This is beark, and causes grievance > > when looking at the code (e.g. to fix it). > > > > Just strip out all leading spaces/tabs when writing the configuration > > lines into the config file. > > > > Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr> > > Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> > > --- > > support/testing/infra/builder.py | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/support/testing/infra/builder.py b/support/testing/infra/builder.py > > index a475bb0a30..81735dec96 100644 > > --- a/support/testing/infra/builder.py > > +++ b/support/testing/infra/builder.py > > @@ -16,7 +16,8 @@ class Builder(object): > > > > config_file = os.path.join(self.builddir, ".config") > > with open(config_file, "w+") as cf: > > - cf.write(self.config) > > + for line in self.config.splitlines(): > > + cf.write("{}\n".format(line.lstrip())) > > I like nitpicking :-) Please nit-pick, python is not my cup of tea, you know well! ;-) > I would do this in BRTest instead of down here, so that self.config contains a > correct config file. In fact, I would do it in its constructor instead of in > setUp(), so that it is done only once even if a class has two test functions. Meh, I have no real clue on how those class- or instance-scoped variables play iwth each others... I think this is sad that we use class-scoped config variable for the actual tests; they should be instance-scoped instead, but that requires one to actually write an __init__ function, so a classscoped is quicker to write. Be if we ever want to derive a Test class into another, then we'll have issues I guess... But if I understand you correctly, you'd prefer (code elided for brevity): class BRTest(): def __init__(self): self.config = '\n'.join(map(str.lstrip, self.config.splitlines())) Right? > I would also do it as a oneliner: > self.config = '\n'.join(map(str.lstrip, self.config.splitlines())) > but some people don't like oneliners :-) Well, I prefer it, because it is rather self-explanatory. But I could not have wrote it myself (I did not know about map() ). Thanks! Regards, Yann E. MORIN.
On 18-07-17 21:45, Yann E. MORIN wrote: > Arnout, All, > > On 2017-07-18 21:37 +0200, Arnout Vandecappelle spake thusly: [snip] >> I would do this in BRTest instead of down here, so that self.config contains a >> correct config file. In fact, I would do it in its constructor instead of in >> setUp(), so that it is done only once even if a class has two test functions. > > Meh, I have no real clue on how those class- or instance-scoped > variables play iwth each others... I think this is sad that we use > class-scoped config variable for the actual tests; they should be > instance-scoped instead, but that requires one to actually write an > __init__ function, so a classscoped is quicker to write. > > Be if we ever want to derive a Test class into another, then we'll have > issues I guess... > > But if I understand you correctly, you'd prefer (code elided for > brevity): > > class BRTest(): > def __init__(self): > self.config = '\n'.join(map(str.lstrip, self.config.splitlines())) > > Right? Exactly. Note that this conveniently converts the class-scoped variable to an instance-scoped one (the reference on the right-hand side refers to the class variable, the reference on the left-hand side creates the instance variable). Still a big Meh for you, but at least after __init__ it feels natural. > >> I would also do it as a oneliner: >> self.config = '\n'.join(map(str.lstrip, self.config.splitlines())) >> but some people don't like oneliners :-) > > Well, I prefer it, because it is rather self-explanatory. But I could > not have wrote it myself (I did not know about map() ). TBH, nowadays Python people prefer the [ for ] construct, so self.config = '\n'.join([line.lstrip for line in self.config.splitlines()]) I'm still an old LISP guy so I know map() better :-) Regards, Arnout
Arnout, All, On 2017-07-18 21:55 +0200, Arnout Vandecappelle spake thusly: > On 18-07-17 21:45, Yann E. MORIN wrote: > > Arnout, All, > > > > On 2017-07-18 21:37 +0200, Arnout Vandecappelle spake thusly: > [snip] > >> I would do this in BRTest instead of down here, so that self.config contains a > >> correct config file. In fact, I would do it in its constructor instead of in > >> setUp(), so that it is done only once even if a class has two test functions. > > > > Meh, I have no real clue on how those class- or instance-scoped > > variables play iwth each others... I think this is sad that we use > > class-scoped config variable for the actual tests; they should be > > instance-scoped instead, but that requires one to actually write an > > __init__ function, so a classscoped is quicker to write. > > > > Be if we ever want to derive a Test class into another, then we'll have > > issues I guess... > > > > But if I understand you correctly, you'd prefer (code elided for > > brevity): > > > > class BRTest(): > > def __init__(self): > > self.config = '\n'.join(map(str.lstrip, self.config.splitlines())) > > > > Right? > > Exactly. > > Note that this conveniently converts the class-scoped variable to an > instance-scoped one (the reference on the right-hand side refers to the class > variable, the reference on the left-hand side creates the instance variable). > Still a big Meh for you, but at least after __init__ it feels natural. Ack. > >> I would also do it as a oneliner: > >> self.config = '\n'.join(map(str.lstrip, self.config.splitlines())) > >> but some people don't like oneliners :-) > > > > Well, I prefer it, because it is rather self-explanatory. But I could > > not have wrote it myself (I did not know about map() ). > > TBH, nowadays Python people prefer the [ for ] construct, so > > self.config = '\n'.join([line.lstrip for line in self.config.splitlines()]) Ah, that was the construct I wanted to write, but for the sake of me, I could not remeber the exact syntax... That's what I'll use, because it is more pythonic (as you also noted). > I'm still an old LISP guy so I know map() better :-) LISP. Meh... A whole lot of insipid and stupid parentehses, that's all I remember of it. ;-] Regards, Yann E. MORIN.
diff --git a/support/testing/infra/builder.py b/support/testing/infra/builder.py index a475bb0a30..81735dec96 100644 --- a/support/testing/infra/builder.py +++ b/support/testing/infra/builder.py @@ -16,7 +16,8 @@ class Builder(object): config_file = os.path.join(self.builddir, ".config") with open(config_file, "w+") as cf: - cf.write(self.config) + for line in self.config.splitlines(): + cf.write("{}\n".format(line.lstrip())) cmd = ["make", "O={}".format(self.builddir),
Currently, defining a config fragment in the runtime test infra requires that the fragment not to be indented. This is beark, and causes grievance when looking at the code (e.g. to fix it). Just strip out all leading spaces/tabs when writing the configuration lines into the config file. Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> --- support/testing/infra/builder.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)