support/tests: allow properly indented config fragment

Message ID 20170715224949.25598-1-yann.morin.1998@free.fr
State Changes Requested
Headers show

Commit Message

Yann E. MORIN July 15, 2017, 10:49 p.m.
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(-)

Comments

Thomas Petazzoni July 16, 2017, 7:18 a.m. | #1
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
Yann E. MORIN July 16, 2017, 8:11 a.m. | #2
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.
Arnout Vandecappelle July 18, 2017, 7:37 p.m. | #3
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),
>
Yann E. MORIN July 18, 2017, 7:45 p.m. | #4
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.
Arnout Vandecappelle July 18, 2017, 7:55 p.m. | #5
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
Yann E. MORIN July 18, 2017, 7:58 p.m. | #6
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.

Patch

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),