[PATCH v3 06/16] iotests/297: Add get_files() function

John Snow posted 16 patches 4 years, 4 months ago
Maintainers: John Snow <jsnow@redhat.com>, Kevin Wolf <kwolf@redhat.com>, Hanna Reitz <hreitz@redhat.com>, Cleber Rosa <crosa@redhat.com>, Eduardo Habkost <ehabkost@redhat.com>
[PATCH v3 06/16] iotests/297: Add get_files() function
Posted by John Snow 4 years, 4 months ago
Split out file discovery into its own method to begin separating out the
"environment setup" and "test execution" phases.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/297 | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index 3e86f788fc..b3a56a3a29 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -21,6 +21,7 @@ import re
 import shutil
 import subprocess
 import sys
+from typing import List
 
 import iotests
 
@@ -56,10 +57,15 @@ def is_python_file(filename: str, directory: str = '.') -> bool:
             return False
 
 
+def get_test_files(directory: str = '.') -> List[str]:
+    named_test_dir = os.path.join(directory, 'tests')
+    named_tests = [f"tests/{entry}" for entry in os.listdir(named_test_dir)]
+    check_tests = set(os.listdir(directory) + named_tests) - set(SKIP_FILES)
+    return list(filter(lambda f: is_python_file(f, directory), check_tests))
+
+
 def run_linters():
-    named_tests = [f'tests/{entry}' for entry in os.listdir('tests')]
-    check_tests = set(os.listdir('.') + named_tests) - set(SKIP_FILES)
-    files = [filename for filename in check_tests if is_python_file(filename)]
+    files = get_test_files()
 
     iotests.logger.debug('Files to be checked:')
     iotests.logger.debug(', '.join(sorted(files)))
-- 
2.31.1


Re: [PATCH v3 06/16] iotests/297: Add get_files() function
Posted by Hanna Reitz 4 years, 4 months ago
On 16.09.21 06:09, John Snow wrote:
> Split out file discovery into its own method to begin separating out the
> "environment setup" and "test execution" phases.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   tests/qemu-iotests/297 | 12 +++++++++---
>   1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
> index 3e86f788fc..b3a56a3a29 100755
> --- a/tests/qemu-iotests/297
> +++ b/tests/qemu-iotests/297
> @@ -21,6 +21,7 @@ import re
>   import shutil
>   import subprocess
>   import sys
> +from typing import List
>   
>   import iotests
>   
> @@ -56,10 +57,15 @@ def is_python_file(filename: str, directory: str = '.') -> bool:
>               return False
>   
>   
> +def get_test_files(directory: str = '.') -> List[str]:
> +    named_test_dir = os.path.join(directory, 'tests')
> +    named_tests = [f"tests/{entry}" for entry in os.listdir(named_test_dir)]
> +    check_tests = set(os.listdir(directory) + named_tests) - set(SKIP_FILES)
> +    return list(filter(lambda f: is_python_file(f, directory), check_tests))

Seeing a filter() makes me immensely happy, but I thought that was 
unpythonic?

Reviewed-by: Hanna Reitz <hreitz@redhat.com>


Re: [PATCH v3 06/16] iotests/297: Add get_files() function
Posted by John Snow 4 years, 4 months ago
On Fri, Sep 17, 2021 at 5:24 AM Hanna Reitz <hreitz@redhat.com> wrote:

> On 16.09.21 06:09, John Snow wrote:
> > Split out file discovery into its own method to begin separating out the
> > "environment setup" and "test execution" phases.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >   tests/qemu-iotests/297 | 12 +++++++++---
> >   1 file changed, 9 insertions(+), 3 deletions(-)
> >
> > diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
> > index 3e86f788fc..b3a56a3a29 100755
> > --- a/tests/qemu-iotests/297
> > +++ b/tests/qemu-iotests/297
> > @@ -21,6 +21,7 @@ import re
> >   import shutil
> >   import subprocess
> >   import sys
> > +from typing import List
> >
> >   import iotests
> >
> > @@ -56,10 +57,15 @@ def is_python_file(filename: str, directory: str =
> '.') -> bool:
> >               return False
> >
> >
> > +def get_test_files(directory: str = '.') -> List[str]:
> > +    named_test_dir = os.path.join(directory, 'tests')
> > +    named_tests = [f"tests/{entry}" for entry in
> os.listdir(named_test_dir)]
> > +    check_tests = set(os.listdir(directory) + named_tests) -
> set(SKIP_FILES)
> > +    return list(filter(lambda f: is_python_file(f, directory),
> check_tests))
>
> Seeing a filter() makes me immensely happy, but I thought that was
> unpythonic?
>
>
Eh, depends on what you're doing, I guess?

The alternative spelling is:
list(f for f in check_tests if is_python_file(f, directory))

... which I guess *is* shorter and skips the lambda. but, I suppose I have
some mild revulsion to seeing "f for f in ..." constructs.
If I ever tell you not to use a filter, feel free to cite this patch and
then just tell me to shut up. I apologize for any inconsistencies in my
style, real or imagined.

--js

Reviewed-by: Hanna Reitz <hreitz@redhat.com>
>
>