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

Remove most use of literal lists of simulators. #200

Merged
merged 3 commits into from
Dec 18, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 2 additions & 5 deletions pocs/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
from warnings import warn

from .utils import config
from pocs import hardware
from .utils.database import PanMongo
from .utils.logger import get_root_logger

Expand Down Expand Up @@ -101,11 +102,7 @@ def __init__(self, *args, **kwargs):

self.__version__ = __version__

if 'simulator' in kwargs:
if 'all' in kwargs['simulator']:
self.config['simulator'] = ['camera', 'mount', 'weather', 'night']
else:
self.config['simulator'] = kwargs['simulator']
self.config['simulator'] = hardware.GetSimulatorNames(config=self.config, kwargs=kwargs)

# Set up connection to database
db = kwargs.get('db', self.config['db']['name'])
Expand Down
57 changes: 57 additions & 0 deletions pocs/hardware.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
"""Information about hardware supported by Panoptes."""

ALL_NAMES = sorted(['camera', 'dome', 'mount', 'night', 'weather'])


def GetAllNames(all_names=ALL_NAMES, without=None):
Copy link
Member

Choose a reason for hiding this comment

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

get_all_names

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

"""
"""
if without:
Copy link
Member

Choose a reason for hiding this comment

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

If you change the default to be without=list() then you don't need to check for the if and you can just do the one line return statement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

return [v for v in all_names if v not in without]
return list(all_names)


def GetSimulatorNames(simulator=None, kwargs=None, config=None):
Copy link
Member

Choose a reason for hiding this comment

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

get_simulator_names

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

"""Returns the names of the simulators to be used in lieu of hardware drivers.

Note that returning a list containing 'X' doesn't mean that the config calls for a driver
of type 'X'; that is up to the code working with the config to create drivers for real or
simulated hardware.

This funciton is intended to be called from PanBase or similar, which receives kwargs that
may include simulator, config or both. For example:
GetSimulatorNames(config=self.config, kwargs=kwargs)
Or:
GetSimulatorNames(simulator=simulator, config=self.config)

The reason this function doesn't just take **kwargs as its sole arg is that we need to allow
Copy link
Member

Choose a reason for hiding this comment

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

I don't doubt this is true but seems odd.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Annoying but true.

for the case where the caller is passing in simulator (or config) twice, once on its own,
and once in the kwargs (which won't be examined). Python doesn't permit a keyword argument
to be passed in twice.

Args:
simulator:
An explicit list of names of hardware to be simulated (i.e. hardware drivers
to be replaced with simulators).
kwargs:
The kwargs passed in to the caller, which is inspected for an arg called 'simulator'.
config:
Dictionary created from pocs.yaml or similar.

Returns:
List of names of the hardware to be simulated.
"""
def ExtractSimulator(d):
Copy link
Member

Choose a reason for hiding this comment

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

extract_simulator

if d:
Copy link
Member

Choose a reason for hiding this comment

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

I keep meaning to mention this in other places, but the more pythonic way (EAFP) would be to:

try:
    return d['simulator']
except KeyError:
    return None

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I decided on yet another approach.

return d.get('simulator')
return None
for simulator in [simulator, ExtractSimulator(kwargs), ExtractSimulator(config)]:
Copy link
Member

Choose a reason for hiding this comment

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

Seems odd to reuse simulator

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed... and what a stupid idea! Clearly a sign that I added the simulator kwarg after deciding that the for loop variable would be called simulator.

if not simulator:
continue
if isinstance(simulator, str):
simulator = [simulator]
if 'all' in simulator:
return GetAllNames()
else:
return sorted(simulator)
return []
44 changes: 17 additions & 27 deletions pocs/tests/conftest.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import os
import pytest

from pocs import hardware
from pocs.utils.config import load_config
from pocs.utils.database import PanMongo

Expand All @@ -16,42 +17,31 @@ def pytest_addoption(parser):
def pytest_collection_modifyitems(config, items):
""" Modify tests to skip or not based on cli options

Certain tests should only be run when the appropriate hardware is attached.
These tests should be marked as follows:
Certain tests should only be run when the appropriate hardware is attached. The names of the
types of hardware are in hardware.py, but include 'mount' and 'camera'. For a test that
requires a mount, for example, the test should be marked as follows:

`@pytest.mark.with_camera`: Run tests with camera attached.
`@pytest.mark.with_mount`: Run tests with mount attached.
`@pytest.mark.with_weather`: Run tests with weather attached.

And the same applies for the names of other types of hardware.

Note:
We are marking which tests to skip rather than which tests to include
so the logic is opposite of the options.
"""

hardware_list = config.getoption('--with-hardware')

if 'all' in hardware_list:
has_camera = True
has_mount = True
has_weather = True
else:
has_camera = 'camera' in hardware_list
has_mount = 'mount' in hardware_list
has_weather = 'weather' in hardware_list

skip_camera = pytest.mark.skip(reason="need --with-hardware=camera option to run")
skip_mount = pytest.mark.skip(reason="need --with-hardware=mount option to run")
skip_weather = pytest.mark.skip(reason="need --with-hardware=weather option to run")

for marker in items:
if "with_camera" in marker.keywords and not has_camera:
marker.add_marker(skip_camera)

if "with_mount" in marker.keywords and not has_mount:
marker.add_marker(skip_mount)

if "with_weather" in marker.keywords and not has_weather:
marker.add_marker(skip_weather)
for name in hardware.GetAllNames():
# Do we have hardware called name?
if name in hardware_list:
# Yes, so don't need to skip tests with keyword "with_name".
continue
# No, so find all the tests that need this type of hardware and mark them to be skipped.
skip = pytest.mark.skip(reason="need --with-hardware={} option to run".format(name))
keyword = 'with_' + name
for item in items:
if keyword in item.keywords:
item.add_marker(skip)


@pytest.fixture
Expand Down
13 changes: 7 additions & 6 deletions pocs/tests/test_observatory.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
from astropy import units as u
from astropy.time import Time

from pocs import hardware
from pocs.observatory import Observatory
from pocs.scheduler.dispatch import Scheduler
from pocs.scheduler.observation import Observation
Expand All @@ -18,7 +19,7 @@ def simulator():
Tests that require real hardware should be marked with the appropriate
fixtue (see `conftest.py`)
"""
return ['camera', 'mount', 'weather']
return hardware.GetAllNames(without=['night'])


@pytest.fixture
Expand Down Expand Up @@ -49,7 +50,7 @@ def test_bad_site(simulator, config):

def test_bad_mount(config):
conf = config.copy()
simulator = ['weather', 'camera', 'night']
simulator = hardware.GetAllNames(without=['mount'])
conf['mount']['port'] = '/dev/'
conf['mount']['driver'] = 'foobar'
with pytest.raises(error.NotFound):
Expand All @@ -74,22 +75,22 @@ def test_bad_scheduler_fields_file(config):

def test_bad_camera(config):
conf = config.copy()
simulator = ['weather', 'mount', 'night']
simulator = hardware.GetAllNames(without=['camera'])
with pytest.raises(error.PanError):
Observatory(simulator=simulator, config=conf, auto_detect=True, ignore_local_config=True)


def test_camera_not_found(config):
conf = config.copy()
simulator = ['weather', 'mount', 'night']
simulator = hardware.GetAllNames(without=['camera'])
with pytest.raises(error.PanError):
Observatory(simulator=simulator, config=conf, ignore_local_config=True)


def test_camera_port_error(config):
conf = config.copy()
conf['cameras']['devices'][0]['model'] = 'foobar'
simulator = ['weather', 'mount', 'night']
simulator = hardware.GetAllNames(without=['camera'])
with pytest.raises(error.CameraNotFound):
Observatory(simulator=simulator, config=conf, auto_detect=False, ignore_local_config=True)

Expand All @@ -98,7 +99,7 @@ def test_camera_import_error(config):
conf = config.copy()
conf['cameras']['devices'][0]['model'] = 'foobar'
conf['cameras']['devices'][0]['port'] = 'usb:001,002'
simulator = ['weather', 'mount', 'night']
simulator = hardware.GetAllNames(without=['camera'])
with pytest.raises(error.NotFound):
Observatory(simulator=simulator, config=conf, auto_detect=False, ignore_local_config=True)

Expand Down
6 changes: 2 additions & 4 deletions pocs/utils/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
import yaml

from astropy import units as u
from pocs import hardware
from pocs.utils import listify
from warnings import warn

Expand Down Expand Up @@ -42,10 +43,7 @@ def load_config(config_files=None, simulator=None, parse=True, ignore_local=Fals
warn("Problem with local config file {}, skipping".format(local_version))

if simulator is not None:
if 'all' in simulator:
config['simulator'] = ['camera', 'mount', 'weather', 'night', 'dome']
else:
config['simulator'] = simulator
config['simulator'] = hardware.GetSimulatorNames(simulator=simulator)

if parse:
config = parse_config(config)
Expand Down
3 changes: 2 additions & 1 deletion scripts/send_home.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
#!/usr/bin/env python3

from pocs import hardware
from pocs import POCS

pocs = POCS(simulator=['camera', 'weather'])
pocs = POCS(simulator=hardware.GetAllNames(without=['mount', 'night']))
pocs.observatory.mount.initialize()
pocs.observatory.mount.home_and_park()
pocs.power_down()