[1/2] support/run-tests: move packages tests to packages directories

Move the existing few packages' tests to the correspnding package
directories.

If we were to add the package/ directory to nose2's search path, we
would have to add __init__.py files about everywhere, at each level of
the directory hierarchy, because nose2 only includes tests that are in a
python pacakge (a directory with a __init__.py file is a python package).
This is far from ideal, especially since we have absolutely nothing to
put in those __init__.py files.

An alternative is to just scan the package/ directory ourselves, and
create symlinks in a sub-directory of support/tessting/tests/ so that
nose2 does find them. We have no two packages with the same name, we
don't risk having two symlinks with the same name.

That second solution is what we choose to do, at the cost of a slight
increase in complexity in the run-test script.

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Ricardo Martincoski <ricardo.martincoski@gmail.com>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Arnout Vandecappelle <arnout@mind.be>
Cc: Samuel Martin <s.martin49@gmail.com>
---
 .gitignore                                         |  1 +
 .../dropbear/dropbear.py                           |  0
 .../python-ipython/python-ipython.py               |  0
 .../test_python.py => package/python/python.py     |  0
 support/testing/run-tests                          | 22 ++++++++++++++++++++++
 support/testing/tests/package/__init__.py          |  0
 6 files changed, 23 insertions(+)
 rename support/testing/tests/package/test_dropbear.py => package/dropbear/dropbear.py (100%)
 rename support/testing/tests/package/test_ipython.py => package/python-ipython/python-ipython.py (100%)
 rename support/testing/tests/package/test_python.py => package/python/python.py (100%)
 delete mode 100644 support/testing/tests/package/__init__.py

diff --git a/support/testing/tests/package/__init__.py b/support/testing/tests/package/__init__.py
deleted file mode 100644
index e69de29bb2..0000000000

Hello,

On Sat, Sep 16, 2017 at 05:38 PM, Yann E. MORIN wrote:

> Move the existing few packages' tests to the correspnding package

typo:                                          corresponding

> directories.
> 
> If we were to add the package/ directory to nose2's search path, we
> would have to add __init__.py files about everywhere, at each level of
> the directory hierarchy, because nose2 only includes tests that are in a
> python pacakge (a directory with a __init__.py file is a python package).

typo:    package

> This is far from ideal, especially since we have absolutely nothing to
> put in those __init__.py files.
> 
> An alternative is to just scan the package/ directory ourselves, and
> create symlinks in a sub-directory of support/tessting/tests/ so that

typo:                                           testing

> nose2 does find them. We have no two packages with the same name, we
> don't risk having two symlinks with the same name.
> 
> That second solution is what we choose to do, at the cost of a slight
> increase in complexity in the run-test script.
> 
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> Cc: Ricardo Martincoski <ricardo.martincoski@gmail.com>
> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: Arnout Vandecappelle <arnout@mind.be>
> Cc: Samuel Martin <s.martin49@gmail.com>
> ---
>  .gitignore                                         |  1 +
>  .../dropbear/dropbear.py                           |  0
>  .../python-ipython/python-ipython.py               |  0
>  .../test_python.py => package/python/python.py     |  0
>  support/testing/run-tests                          | 22 ++++++++++++++++++++++
>  support/testing/tests/package/__init__.py          |  0
>  6 files changed, 23 insertions(+)
>  rename support/testing/tests/package/test_dropbear.py => package/dropbear/dropbear.py (100%)
>  rename support/testing/tests/package/test_ipython.py => package/python-ipython/python-ipython.py (100%)

.gitlab-ci.yml needs to be generated again because of this rename.

As I mentioned in another thread, having Python files with '-' in their name
AFAIK is not a problem as long we don't want to import them.
We currently import only test_python.py and it seems unlikely to have runtime
tests importing each other.

And this series is really changing the filename convention for runtime tests of
packages, making the pre-existing convention of package files beat the
convention inside the test infra. I am OK with this change, but I think it worth
mentioning. Perhaps it fits in the commit log?

[snip]
> @@ -36,6 +39,9 @@ def main():
>      script_path = os.path.realpath(__file__)
>      test_dir = os.path.dirname(script_path)
>  
> +    gatheradditionaltests(os.path.abspath("."),

Below you run all files in this path (base dir of buildroot) to match against
the regex. It's bad because one can have output/, test-output/, test-dl/,
outgoing/, ...

Also os.getcwd() should give the same result and it is already used in 
support/testing/infra/__init__.py

So for now I suggest to change to:
gather_additional_tests(os.path.join(os.getcwd(), "package"),
And if fs tests gets moved, just add another call to the same method.
We don't have that many subdirectories in the root directory.

Other way around is to pass an array of relative subdirectories
gather_additional_tests(["package", "fs"],
and compose the absolute path inside the method while iterating over the
entries.

> +                          os.path.join(test_dir,"tests","package"))

Here and all over the file there are missing whitespace after ','
Can you use flake8 to find them and fix it?

If you use vim you can do
:set makeprg=flake8\ %
:make
and it will jump the cursor to the right place

> +
>      if args.stdout:
>          BRTest.logtofile = False
>  
> @@ -116,5 +122,21 @@ def main():
>  
>      nose2.discover(argv=nose2_args)
>  
> +def gatheradditionaltests(search_dir, symlink_dir):

You could call it gather_additional_tests.

> +    try:
> +        shutil.rmtree(symlink_dir)
> +    except OSError as err:
> +        if not err.errno == errno.ENOENT:
> +            raise err
> +        pass

Could we do just this? (like is done in builder.py)
    if os.path.exists(symlink_dir):
        shutil.rmtree(symlink_dir)
If you change this, errno doesn't need to be imported.

> +    os.mkdir(symlink_dir)
> +    open(os.path.join(symlink_dir,"__init__.py"), "w").close()
> +    for dir, _, files in os.walk(os.path.abspath(search_dir)):

I first thought, why not glob.glob? But in Python 2 it is not recursive.
So indeed os.walk is the way to go.

> +        package = os.path.basename(dir)
> +        for file in files:

You can import fnmatch to drastically reduce the number of regex compilations:
        for file in fnmatch.filter(files, '*.py'):

> +            if re.match("^{}.py$".format(package),file):
> +                os.symlink(os.path.join(dir,file),
> +                           os.path.join(symlink_dir,"test_{}.py".format(package)))

Regards,
Ricardo

Ricardo, All,

On 2017-09-17 02:20 -0300, Ricardo Martincoski spake thusly:
> On Sat, Sep 16, 2017 at 05:38 PM, Yann E. MORIN wrote:
> > ---
> >  .gitignore                                         |  1 +
> >  .../dropbear/dropbear.py                           |  0
> >  .../python-ipython/python-ipython.py               |  0
> >  .../test_python.py => package/python/python.py     |  0
> >  support/testing/run-tests                          | 22 ++++++++++++++++++++++
> >  support/testing/tests/package/__init__.py          |  0
> >  6 files changed, 23 insertions(+)
> >  rename support/testing/tests/package/test_dropbear.py => package/dropbear/dropbear.py (100%)
> >  rename support/testing/tests/package/test_ipython.py => package/python-ipython/python-ipython.py (100%)
> As I mentioned in another thread, having Python files with '-' in their name
> AFAIK is not a problem as long we don't want to import them.
> We currently import only test_python.py and it seems unlikely to have runtime
> tests importing each other.

Yet we don't know what the future will be made of, so we should squash
the dasj into an underscore, so as to allow this kind of situation.

> And this series is really changing the filename convention for runtime tests of
> packages, making the pre-existing convention of package files beat the
> convention inside the test infra. I am OK with this change, but I think it worth
> mentioning. Perhaps it fits in the commit log?

Yes, it does change the naming convention. I was about to add it to our
manual, in the section about adding new packages, to document the .py
file in addition to Config.in and the .mk and .hash files. But then I
noticed we had nothing about the runtime tests in the manual, so I could
not refer to that section to explain the .py file.

So I deferred updating the manual for later.

> [snip]
> > @@ -36,6 +39,9 @@ def main():
> >      script_path = os.path.realpath(__file__)
> >      test_dir = os.path.dirname(script_path)
> >  
> > +    gatheradditionaltests(os.path.abspath("."),
> 
> Below you run all files in this path (base dir of buildroot) to match against
> the regex. It's bad because one can have output/, test-output/, test-dl/,
> outgoing/, ...

Arg, I initially wanted to only scan package/ in fact.

> Also os.getcwd() should give the same result and it is already used in 
> support/testing/infra/__init__.py
> 
> So for now I suggest to change to:
> gather_additional_tests(os.path.join(os.getcwd(), "package"),

Except that both your solution and mine break when the script is not
called from the Buildroot top directory. We must derive the top
directory from the script path.

> And if fs tests gets moved, just add another call to the same method.
> We don't have that many subdirectories in the root directory.
> 
> Other way around is to pass an array of relative subdirectories
> gather_additional_tests(["package", "fs"],
> and compose the absolute path inside the method while iterating over the
> entries.

I think the origin directory should be passed as argument to the
function, for the above reason.

And yes, we could pass a list of directories.

> > +                          os.path.join(test_dir,"tests","package"))
> 
> Here and all over the file there are missing whitespace after ','
> Can you use flake8 to find them and fix it?

Yup, will do.

> If you use vim you can do
> :set makeprg=flake8\ %
> :make
> and it will jump the cursor to the right place
> 
> > +
> >      if args.stdout:
> >          BRTest.logtofile = False
> >  
> > @@ -116,5 +122,21 @@ def main():
> >  
> >      nose2.discover(argv=nose2_args)
> >  
> > +def gatheradditionaltests(search_dir, symlink_dir):
> 
> You could call it gather_additional_tests.

I never know whatThe_prefered_Solutionis. ;-)

> > +    try:
> > +        shutil.rmtree(symlink_dir)
> > +    except OSError as err:
> > +        if not err.errno == errno.ENOENT:
> > +            raise err
> > +        pass
> 
> Could we do just this? (like is done in builder.py)
>     if os.path.exists(symlink_dir):
>         shutil.rmtree(symlink_dir)
> If you change this, errno doesn't need to be imported.

So I am no Python expert, and this shows. Thanks for the hints! :-)

> > +    os.mkdir(symlink_dir)
> > +    open(os.path.join(symlink_dir,"__init__.py"), "w").close()
> > +    for dir, _, files in os.walk(os.path.abspath(search_dir)):
> 
> I first thought, why not glob.glob? But in Python 2 it is not recursive.
> So indeed os.walk is the way to go.
> 
> > +        package = os.path.basename(dir)
> > +        for file in files:
> 
> You can import fnmatch to drastically reduce the number of regex compilations:
>         for file in fnmatch.filter(files, '*.py'):

Wee! :-)

> > +            if re.match("^{}.py$".format(package),file):
> > +                os.symlink(os.path.join(dir,file),
> > +                           os.path.join(symlink_dir,"test_{}.py".format(package)))
> 
> Regards,
> Ricardo

Ricardo, All,

On 2017-09-17 09:18 +0200, Yann E. MORIN spake thusly:
> On 2017-09-17 02:20 -0300, Ricardo Martincoski spake thusly:
> > On Sat, Sep 16, 2017 at 05:38 PM, Yann E. MORIN wrote:
[--SNIP--]
> > > +        package = os.path.basename(dir)
> > > +        for file in files:
> > 
> > You can import fnmatch to drastically reduce the number of regex compilations:
> >         for file in fnmatch.filter(files, '*.py'):
> Wee! :-)

In fact, no. At least, not in this state, because we do not want to
match _any_ .py file; instead, we want to match only .py files that are
named after the package.

But I guess I can make it to work with fnmatch nonetheless. ;-)

Regards,
Yann E. MORIN.

Hello,

On Sun, Sep 17, 2017 at 04:52 AM, Yann E. MORIN wrote:

> On 2017-09-17 09:18 +0200, Yann E. MORIN spake thusly:
>> On 2017-09-17 02:20 -0300, Ricardo Martincoski spake thusly:
>> > On Sat, Sep 16, 2017 at 05:38 PM, Yann E. MORIN wrote:
> [--SNIP--]
>> > > +        package = os.path.basename(dir)
>> > > +        for file in files:
>> > 
>> > You can import fnmatch to drastically reduce the number of regex compilations:
>> >         for file in fnmatch.filter(files, '*.py'):
>> Wee! :-)
> 
> In fact, no. At least, not in this state, because we do not want to
> match _any_ .py file; instead, we want to match only .py files that are
> named after the package.

We could use 2 filters. Instead of 8k+ regex compilations (one for each file
inside a package directory) we would run 2k+ fnmatch.filter (that probably
compile one regex, one per directory) and one regex compilation for each test
file in the package tree.

         for file in fnmatch.filter(files, '*.py'):
             if re.match("^{}.py$".format(package),file):

But well... the default python interpreter has a cache for regex (I don't know
its internals, just that it exists) and perhaps the change I suggested won't
make any difference in performance.

> 
> But I guess I can make it to work with fnmatch nonetheless. ;-)

BTW, could you change the DEVELOPERS file in the same patch?
+F:     package/*/*.py
 F:     support/testing/

I know it won't work for all possible files. 99% is enough for me.

Regards,
Ricardo

Ricardo, All,

On 2017-10-01 21:54 -0300, Ricardo Martincoski spake thusly:
> On Sun, Sep 17, 2017 at 04:52 AM, Yann E. MORIN wrote:
> > On 2017-09-17 09:18 +0200, Yann E. MORIN spake thusly:
> >> On 2017-09-17 02:20 -0300, Ricardo Martincoski spake thusly:
> >> > On Sat, Sep 16, 2017 at 05:38 PM, Yann E. MORIN wrote:
> > [--SNIP--]
> >> > > +        package = os.path.basename(dir)
> >> > > +        for file in files:
> >> > 
> >> > You can import fnmatch to drastically reduce the number of regex compilations:
> >> >         for file in fnmatch.filter(files, '*.py'):
> >> Wee! :-)
> > 
> > In fact, no. At least, not in this state, because we do not want to
> > match _any_ .py file; instead, we want to match only .py files that are
> > named after the package.
> 
> We could use 2 filters. Instead of 8k+ regex compilations (one for each file
> inside a package directory) we would run 2k+ fnmatch.filter (that probably
> compile one regex, one per directory) and one regex compilation for each test
> file in the package tree.
> 
>          for file in fnmatch.filter(files, '*.py'):
>              if re.match("^{}.py$".format(package),file):

Err... I already changed it to:

    for file in fnmatch.filter(files, '{}.py'.format(package)):

Is there something wrong with that? ;-)

> But well... the default python interpreter has a cache for regex (I don't know
> its internals, just that it exists) and perhaps the change I suggested won't
> make any difference in performance.
> 
> > 
> > But I guess I can make it to work with fnmatch nonetheless. ;-)
> 
> BTW, could you change the DEVELOPERS file in the same patch?
> +F:     package/*/*.py
>  F:     support/testing/

You meant, add it to your entry ?

In fact, I would expect to assign the test files to the corresponding
package custodian (now imp[licit by their location).

But yes, I can assign them to you as well.

Regards,
Yann E. MORIN.

Hello,

On Mon, Oct 02, 2017 at 02:32 AM, Yann E. MORIN wrote:

> On 2017-10-01 21:54 -0300, Ricardo Martincoski spake thusly:
>> On Sun, Sep 17, 2017 at 04:52 AM, Yann E. MORIN wrote:
>> > On 2017-09-17 09:18 +0200, Yann E. MORIN spake thusly:
>> >> On 2017-09-17 02:20 -0300, Ricardo Martincoski spake thusly:
>> >> > On Sat, Sep 16, 2017 at 05:38 PM, Yann E. MORIN wrote:
>> > [--SNIP--]
>> >> > > +        package = os.path.basename(dir)
>> >> > > +        for file in files:
>> >> > 
>> >> > You can import fnmatch to drastically reduce the number of regex compilations:
>> >> >         for file in fnmatch.filter(files, '*.py'):
>> >> Wee! :-)
>> > 
>> > In fact, no. At least, not in this state, because we do not want to
>> > match _any_ .py file; instead, we want to match only .py files that are
>> > named after the package.
>> 
>> We could use 2 filters. Instead of 8k+ regex compilations (one for each file
>> inside a package directory) we would run 2k+ fnmatch.filter (that probably
>> compile one regex, one per directory) and one regex compilation for each test
>> file in the package tree.
>> 
>>          for file in fnmatch.filter(files, '*.py'):
>>              if re.match("^{}.py$".format(package),file):
> 
> Err... I already changed it to:
> 
>     for file in fnmatch.filter(files, '{}.py'.format(package)):
> 
> Is there something wrong with that? ;-)

Nothing wrong. It's better this (your) way IMO.

> 
>> But well... the default python interpreter has a cache for regex (I don't know
>> its internals, just that it exists) and perhaps the change I suggested won't
>> make any difference in performance.
>> 
>> > 
>> > But I guess I can make it to work with fnmatch nonetheless. ;-)
>> 
>> BTW, could you change the DEVELOPERS file in the same patch?
>> +F:     package/*/*.py
>>  F:     support/testing/
> 
> You meant, add it to your entry ?

Yes.

> 
> In fact, I would expect to assign the test files to the corresponding
> package custodian (now imp[licit by their location).
> 
> But yes, I can assign them to you as well.

Thanks.

Regards,
Ricardo