Libguestfs - May 2022 - [PATCH nbdkit] s3: Only run the tests if boto3 is installed

On 05/17/22 10:55, Richard W.M. Jones wrote:
> ERROR: S3 (unittest.loader._FailedTest)
> ----------------------------------------------------------------------
> ImportError: Failed to import test module: S3
> Traceback (most recent call last):
>   File "/usr/lib64/python3.10/unittest/loader.py", line 154, in
loadTestsFromName
>     module = __import__(module_name)
>   File "/home/rjones/d/nbdkit/plugins/S3/S3.py", line 41, in
<module>
>     import boto3
> ModuleNotFoundError: No module named 'boto3'
> 
> Reported-by: Laszlo Ersek
> Fixes: commit 5130c43bc1f9
> ---
>  tests/test-S3-unit.sh | 1 +
>  tests/test-S3.sh      | 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/tests/test-S3-unit.sh b/tests/test-S3-unit.sh
> index 6b6adf02..8a32dd4c 100755
> --- a/tests/test-S3-unit.sh
> +++ b/tests/test-S3-unit.sh
> @@ -37,6 +37,7 @@ set -x
>  
>  requires $PYTHON --version
>  requires $PYTHON -c 'import unittest'
> +requires $PYTHON -c 'import boto3'
>  
>  # Python has proven very difficult to valgrind, therefore it is disabled.
>  if [ "$NBDKIT_VALGRIND" = "1" ]; then
> diff --git a/tests/test-S3.sh b/tests/test-S3.sh
> index de94751e..e51ef2ac 100755
> --- a/tests/test-S3.sh
> +++ b/tests/test-S3.sh
> @@ -36,6 +36,7 @@ set -x
>  
>  requires hexdump --version
>  requires $PYTHON --version
> +requires $PYTHON -c 'import boto3'
>  requires nbdcopy --version
>  requires_plugin python
>  
> 
This was my very first idea too, but does it not conflict with having a
fake boto3 module too, at "tests/test-S3/boto3"?

In particular, "tests/test-S3.sh" already contains:

  # There is a fake boto3 module in test-S3/ which we use as a test
  # harness for the plugin.
  requires test -d test-S3
  export PYTHONPATH=$srcdir/test-S3:$PYTHONPATH

In other words, if you added the new line

  requires $PYTHON -c 'import boto3'

to the same file, but under the PYTHONPATH extension, then I *think* the
new "requires" would succeed, but the test case would fail the same
way
-- because the fake module does not expose the new exception type.

Of course I'm also 100% fine if we make both test cases dependent on the
*real* boto3 module, but should we then eliminate the "test-S3"
directory? (The fake module, that is?)

Thanks
Laszlo

On Tue, May 17, 2022 at 11:53:44AM +0200, Laszlo Ersek
wrote:
> On 05/17/22 10:55, Richard W.M. Jones wrote:
> > ERROR: S3 (unittest.loader._FailedTest)
> > ----------------------------------------------------------------------
> > ImportError: Failed to import test module: S3
> > Traceback (most recent call last):
> >   File "/usr/lib64/python3.10/unittest/loader.py", line 154,
in loadTestsFromName
> >     module = __import__(module_name)
> >   File "/home/rjones/d/nbdkit/plugins/S3/S3.py", line 41, in
<module>
> >     import boto3
> > ModuleNotFoundError: No module named 'boto3'
> > 
> > Reported-by: Laszlo Ersek
> > Fixes: commit 5130c43bc1f9
> > ---
> >  tests/test-S3-unit.sh | 1 +
> >  tests/test-S3.sh      | 1 +
> >  2 files changed, 2 insertions(+)
> > 
> > diff --git a/tests/test-S3-unit.sh b/tests/test-S3-unit.sh
> > index 6b6adf02..8a32dd4c 100755
> > --- a/tests/test-S3-unit.sh
> > +++ b/tests/test-S3-unit.sh
> > @@ -37,6 +37,7 @@ set -x
> >  
> >  requires $PYTHON --version
> >  requires $PYTHON -c 'import unittest'
> > +requires $PYTHON -c 'import boto3'
> >  
> >  # Python has proven very difficult to valgrind, therefore it is
disabled.
> >  if [ "$NBDKIT_VALGRIND" = "1" ]; then
> > diff --git a/tests/test-S3.sh b/tests/test-S3.sh
> > index de94751e..e51ef2ac 100755
> > --- a/tests/test-S3.sh
> > +++ b/tests/test-S3.sh
> > @@ -36,6 +36,7 @@ set -x
> >  
> >  requires hexdump --version
> >  requires $PYTHON --version
> > +requires $PYTHON -c 'import boto3'
> >  requires nbdcopy --version
> >  requires_plugin python
> >  
> > 
> 
> This was my very first idea too, but does it not conflict with having a
> fake boto3 module too, at "tests/test-S3/boto3"?
Yeah I guess the real problem is that the mocked boto3 isn't working
for some reason.  FWIW this works (with mocked boto3) in nbdkit 1.30.
> In particular, "tests/test-S3.sh" already contains:
> 
>   # There is a fake boto3 module in test-S3/ which we use as a test
>   # harness for the plugin.
>   requires test -d test-S3
>   export PYTHONPATH=$srcdir/test-S3:$PYTHONPATH
> 
> In other words, if you added the new line
> 
>   requires $PYTHON -c 'import boto3'
> 
> to the same file, but under the PYTHONPATH extension, then I *think* the
> new "requires" would succeed, but the test case would fail the
same way
> -- because the fake module does not expose the new exception type.
> 
> Of course I'm also 100% fine if we make both test cases dependent on
the
> *real* boto3 module, but should we then eliminate the "test-S3"
> directory? (The fake module, that is?)
So I think we shouldn't ever be using the real boto3 module, because
that might mean we try to connect to an S3 instance.

Let's ditch this patch and I'll have another look.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

OK I see what's going on.

test-S3.sh uses the mocked boto3 in tests/test-S3/ to do an end-to-end
test (nbdcopy).

test-S3-unit.sh runs the unit tests within the plugin.  It's basically
testing the plugin as if it was a standalone Python script (without
nbdkit being involved).  This uses an internal MockS3Client class also
inside the plugin.

The second one does actually need real boto3, just because there's an
"import boto3" at the top and we don't set up PYTHONPATH to pick
up
the tests/test-S3/ subdirectory.

We could either fix the second one by skipping it if (real) boto3
isn't installed, or by making it use the mocked boto3 from tests/test-S3/

Either approach would work, but the second one (below) seems maybe
better because we'd get more test coverage this way?

Rich.


diff --git a/tests/test-S3-unit.sh b/tests/test-S3-unit.sh
index 6b6adf02..11718be3 100755
--- a/tests/test-S3-unit.sh
+++ b/tests/test-S3-unit.sh
@@ -44,6 +44,6 @@ if [ "$NBDKIT_VALGRIND" = "1" ]; then
     exit 77
 fi
 
-export PYTHONPATH=$srcdir/../plugins/S3:$PYTHONPATH
+export PYTHONPATH=$srcdir/../plugins/S3:$srcdir/test-S3:$PYTHONPATH
 
 $PYTHON -m unittest S3


-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW