Unit tests improperly mocking mkdtemp

Bug #1714306 reported by Ben Nemec
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
tripleo
Fix Released
High
Ben Nemec

Bug Description

This caused the following failure in a unit test job:

2017-08-31 04:19:50.942058 | FAIL: tripleo_common.tests.actions.test_logging_to_swift.PrepareLogDownloadActionTest.test_run_success
2017-08-31 04:19:50.942070 | tags: worker-7
2017-08-31 04:19:50.942098 | ----------------------------------------------------------------------
2017-08-31 04:19:50.942116 | Traceback (most recent call last):
2017-08-31 04:19:50.942164 | File "/home/jenkins/workspace/gate-tripleo-common-python35/.tox/py35/lib/python3.5/site-packages/mock/mock.py", line 1305, in patched
2017-08-31 04:19:50.942182 | return func(*args, **keywargs)
2017-08-31 04:19:50.942232 | File "/home/jenkins/workspace/gate-tripleo-common-python35/tripleo_common/tests/actions/test_logging_to_swift.py", line 143, in test_run_success
2017-08-31 04:19:50.942248 | action.run(self.ctx)
2017-08-31 04:19:50.942315 | File "/home/jenkins/workspace/gate-tripleo-common-python35/tripleo_common/actions/logging_to_swift.py", line 181, in run
2017-08-31 04:19:50.942347 | shutil.rmtree(tmp_dir)
2017-08-31 04:19:50.942391 | File "/home/jenkins/workspace/gate-tripleo-common-python35/.tox/py35/lib/python3.5/shutil.py", line 465, in rmtree
2017-08-31 04:19:50.942412 | onerror(os.lstat, path, sys.exc_info())
2017-08-31 04:19:50.942453 | File "/home/jenkins/workspace/gate-tripleo-common-python35/.tox/py35/lib/python3.5/shutil.py", line 463, in rmtree
2017-08-31 04:19:50.942469 | orig_st = os.lstat(path)
2017-08-31 04:19:50.942497 | FileNotFoundError: [Errno 2] No such file or directory: '/tmp/test123'

I'm not entirely sure how it ever works, but for some reason it's not failing all the time. We shouldn't be mocking mkdtemp in unit tests anyway though - we already have NestedTempfile to clean up any tempfiles we create, and forcing a write to a known system location is probably a security issue too.

Ben Nemec (bnemec)
tags: added: tripleo-common
removed: tripleo-comm
Changed in tripleo:
assignee: nobody → Ben Nemec (bnemec)
status: Triaged → In Progress
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to tripleo-common (master)

Reviewed: https://review.openstack.org/499708
Committed: https://git.openstack.org/cgit/openstack/tripleo-common/commit/?id=0be202dcd102ec06c78059472912938175cc5740
Submitter: Jenkins
Branch: master

commit 0be202dcd102ec06c78059472912938175cc5740
Author: Ben Nemec <email address hidden>
Date: Thu Aug 31 16:29:30 2017 +0000

    Don't mock mkdtemp

    As far as I can tell there's no reason to do this, and it breaks
    the ability for NestedTempfile to clean up after the tests by
    forcing the file to go to the system /tmp directory (which I'm
    pretty sure is a security issue too...). It's also causing some
    weird errors in the gate so let's remove it.

    Change-Id: I04f3f569ff904e002758098cfd90b5bbb31c6afc
    Closes-Bug: 1714306

Changed in tripleo:
status: In Progress → Fix Released
Revision history for this message
Honza Pokorny (hpokorny) wrote :

What about the other instances in the test suite? e.g.

https://github.com/openstack/tripleo-common/blob/master/tripleo_common/tests/actions/test_plan.py#L305

I don't really understand the issue but it seems weird that it's fine in some places and not others.

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to tripleo-common (stable/pike)

Fix proposed to branch: stable/pike
Review: https://review.openstack.org/500083

Revision history for this message
Ben Nemec (bnemec) wrote :

You're right, we shouldn't be doing it there either. I just fixed the one that caused the ci job failure. I'll push another patch to remove that one too.

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to tripleo-common (master)

Fix proposed to branch: master
Review: https://review.openstack.org/500129

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to tripleo-common (stable/pike)

Reviewed: https://review.openstack.org/500083
Committed: https://git.openstack.org/cgit/openstack/tripleo-common/commit/?id=9a6bcf5ab602258c3caa30d57d242d33e6fb79b9
Submitter: Jenkins
Branch: stable/pike

commit 9a6bcf5ab602258c3caa30d57d242d33e6fb79b9
Author: Ben Nemec <email address hidden>
Date: Thu Aug 31 16:29:30 2017 +0000

    Don't mock mkdtemp

    As far as I can tell there's no reason to do this, and it breaks
    the ability for NestedTempfile to clean up after the tests by
    forcing the file to go to the system /tmp directory (which I'm
    pretty sure is a security issue too...). It's also causing some
    weird errors in the gate so let's remove it.

    Change-Id: I04f3f569ff904e002758098cfd90b5bbb31c6afc
    Closes-Bug: 1714306
    (cherry picked from commit 0be202dcd102ec06c78059472912938175cc5740)

tags: added: in-stable-pike
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to tripleo-common (master)

Reviewed: https://review.openstack.org/500129
Committed: https://git.openstack.org/cgit/openstack/tripleo-common/commit/?id=2edc35c840a1b2eb133dd4775a158c61b12924a8
Submitter: Jenkins
Branch: master

commit 2edc35c840a1b2eb133dd4775a158c61b12924a8
Author: Ben Nemec <email address hidden>
Date: Fri Sep 1 16:44:44 2017 +0000

    Don't mock mkdtemp in test_plan.py either

    Same reasoning as I04f3f569ff904e002758098cfd90b5bbb31c6afc.

    Change-Id: I3be7c8e315f1e765c0512619f04a5cb4bc9bb6fc
    Closes-Bug: 1714306

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix included in openstack/tripleo-common 7.6.0

This issue was fixed in the openstack/tripleo-common 7.6.0 release.

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix included in openstack/tripleo-common 8.0.0

This issue was fixed in the openstack/tripleo-common 8.0.0 release.

To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.