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

changes integration tests #64

Closed
wants to merge 2 commits into from
Closed

changes integration tests #64

wants to merge 2 commits into from

Conversation

sjovdnbos
Copy link
Contributor

-api usage changes
-bugfixes

-api usage changes
-bugfixes
class DiskHelper(object):
"""
DiskHelper class
"""

def __init__(self):
from ..helpers.storagerouter import StoragerouterHelper
Copy link
Contributor

Choose a reason for hiding this comment

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

Unused import

@@ -45,14 +46,36 @@ def __eq__(self, o):
def __str__(self):
return "{} {} {} {} {} {}".format(self.device, self.mountpoint, self.filesystem, self.options, self.d, self.p)

def get(self, item):
if not isinstance(item,basestring):
Copy link
Contributor

Choose a reason for hiding this comment

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

Spacing

self.truncate()
return True
with open(cls._path, 'r+') as fh:
d = fh.readlines()
Copy link
Contributor

Choose a reason for hiding this comment

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

Use some meaningfull names here



class FstabHelper(file):
class FstabHelper():
Copy link
Contributor

Choose a reason for hiding this comment

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

Inherit from object instead of nothing



class FstabHelper(file):
class FstabHelper():
"""
Class to help with Fstab manipulations
Inherits from file class
"""
import os
Copy link
Contributor

Choose a reason for hiding this comment

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

Imports belong to the top

@staticmethod
def create_snapshot(snapshot_name, vdisk_name, vpool_name, api, consistent=True, sticky=True,
@classmethod
def create_snapshot(snapshot_name, vdisk_name, vpool_name, consistent=True, sticky=True,
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing cls declaration. Snapshot_name is currently treated as the VDiskSetup pointer

@required_vdisk
def set_vdisk_as_template(vdisk_name, vpool_name, api, timeout=SET_VDISK_AS_TEMPLATE_TIMEOUT):
def set_vdisk_as_template(vdisk_name, vpool_name, timeout=SET_VDISK_AS_TEMPLATE_TIMEOUT):
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing cls declaration. Snapshot_name is currently treated as the VDiskSetup pointer

@required_vdisk
def set_config_params(vdisk_name, vpool_name, config, api, timeout=SET_CONFIG_VDISK_TIMEOUT):
def set_config_params(vdisk_name, vpool_name, config, timeout=SET_CONFIG_VDISK_TIMEOUT):
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing cls declaration. Snapshot_name is currently treated as the VDiskSetup pointer

@@ -31,19 +33,20 @@ def check_required_roles(roles, storagerouter_ip=None, location="GLOBAL"):

:param roles: the required roles
:type roles: list
:param storagerouter_ip: ip address of a storagerouter
:param storagerouter_ip: guid of a storagerouter
Copy link
Contributor

Choose a reason for hiding this comment

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

Nope, still an ip

# fetch availabe roles
if location == "LOCAL":
# LOCAL
available_roles = DiskHelper.get_roles_from_disks(storagerouter_ip=storagerouter_ip)
storagerouter_guid = StoragerouterHelper.get_storagerouter_by_ip(storagerouter_ip).guid

Copy link
Contributor

Choose a reason for hiding this comment

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

Why spacing here 🐙

@@ -13,32 +13,30 @@
#
# Open vStorage is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY of any kind.
from ci.scenario_helpers.ci_constants import CIConstants
Copy link
Contributor

Choose a reason for hiding this comment

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

We might have glossed over something completely here.
The idea was that the automation library would be completely independent from anything (currently not the case as lookups are still happening by using the DAL instead of the API, #57 would remove this dependency)
Now with these imports, the integration tests have to be installed together with the automation_lib in order for it to work. We do not want this.
Instead I propose the following:

  • Move the CIConstants api part to the automation lib (let it read from the same file)
  • Strip the integration test stuff from CIConstants in automation lib
  • Create a new CIConstants inside the integration tests which inherit the code from api_lib
  • Apply the required stuff for integration tests in the new class

@sjovdnbos sjovdnbos closed this Nov 23, 2017
@sjovdnbos sjovdnbos deleted the api_changes branch November 23, 2017 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants