Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update google storage #601

Merged
merged 13 commits into from
Sep 20, 2018
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
111 changes: 111 additions & 0 deletions pocs/tests/utils/google/test_storage.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
import pytest
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add some comments please?

import os
import re
import shutil

from pocs.utils.config import load_config
from pocs.utils.error import GoogleCloudError
from pocs.utils.google.storage import PanStorage, upload_observation_to_bucket


pytestmark = pytest.mark.skipif(
not pytest.config.option.test_cloud_storage,
reason="Needs --test-cloud-storage to run"
)


@pytest.fixture(scope="module")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason to not make this a function scope fixture and have it take the existing config fixture as a param?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.

def auth_key():
local_config = load_config('pocs_local', ignore_local=True)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why pocs_local AND ignore_local? If intended to test load_config, then I think the test belongs elsewhere.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We now rely on authentication being done for the system as a whole. As such, I am removing the ability to to pass an auth_key to the PanStorage object directly. That also then makes some of this testing infrastructure disappear.

auth_key = os.path.join(
os.environ['PANDIR'],
'.keys',
local_config['panoptes_network']['auth_key']
)

return auth_key


def test_key_exists(auth_key):
assert os.path.exists(auth_key), "No API key file"


def test_bad_bucket(auth_key):
with pytest.raises(AssertionError):
PanStorage('fake-bucket')

with pytest.raises(GoogleCloudError):
PanStorage('fake-bucket', auth_key=auth_key)


@pytest.fixture(scope="function")
def storage(auth_key):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't help but think that there should be a way that we can have an auth_key that is not in github for all the world to see, but that can be used by each tester for accessing the test bucket without having to use the same credentials that give access to the primary bucket. Of course, maybe you've already addressed this.

If not, travis-ci does support secret values being passed into tests.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is a generic testing auth_key. I saw reference to that somewhere, will try to find.

return PanStorage('panoptes-test-bucket', auth_key=auth_key)


def test_unit_id(storage):
assert storage.unit_id is not None
# TODO(wtgee)Verify the unit id better after #384 is done.
assert re.match(r'PAN\d\d\d', storage.unit_id), storage.logger.error(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is being tested here? Shouldn't we just ensure for testing that we provide a config that has the correct contents?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, test removed as not appropriate.

"Must have valid pan_id. Please change your conf_files/pocs_local.yaml")


def test_bucket_exists(storage):
assert storage.bucket.exists()


def test_file_upload_no_prepend(storage):
temp_fn = 'ping.txt'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fixed names don't allow for multiple runs of this test concurrently (e.g. by travis.ci, you, Anthony and me).

with open(temp_fn, 'w') as f:
f.write('Hello World')

remote_path = storage.upload_file(temp_fn)
assert remote_path == os.path.join(storage.unit_id, temp_fn)
assert storage.bucket.blob(remote_path).exists()
os.unlink(temp_fn)


def test_file_upload_prepend_remote_path(storage):
temp_fn = 'pong.txt'.format(storage.unit_id)
with open(temp_fn, 'w') as f:
f.write('Hello World')

remote_path = os.path.join(storage.unit_id, temp_fn)
returned_remote_path = storage.upload_file(temp_fn, remote_path=remote_path)
assert remote_path == returned_remote_path
assert storage.bucket.blob(returned_remote_path).exists()
os.unlink(temp_fn)


def test_delete(storage):
"""
Note: The util wrappers don't provide a way to delete because we generally
will not want people to delete things. However it's good to test and we
want to remove the above files
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both files aren't removed.

"""
remote_path = '{}/pong.txt'.format(storage.unit_id)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd very much like to avoid depending on tests running serially and in a specific order, unless there is a fixture that is used to ensure that happens. I sometimes run using the xdist or random plugins in order to try find mistakes where our tests are non-hermetic.

assert storage.bucket.blob(remote_path).exists()
storage.bucket.blob(remote_path).delete()


def test_upload_observation_to_bucket(storage):
dir_name = os.path.join(
os.environ['POCS'],
'pocs', 'tests', 'data'
)

# We copy all the files to a temp dir and then upload that to get
# correct pathnames.
new_dir = os.path.join('/tmp', 'fields', 'fake_obs')
shutil.copytree(dir_name, new_dir)

include_files = '*'

pan_id = storage.unit_id
assert upload_observation_to_bucket(
pan_id,
new_dir,
include_files=include_files,
bucket='panoptes-test-bucket')

shutil.rmtree(new_dir, ignore_errors=True)
77 changes: 0 additions & 77 deletions pocs/tests/utils/test_google_storage.py

This file was deleted.

Loading