-
Notifications
You must be signed in to change notification settings - Fork 3
58 CI automation of test suite, take two #160
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
base: master
Are you sure you want to change the base?
Changes from all commits
f6f6992
e0ad696
c9096ce
0ed80b7
f633eb3
340a2b0
68109c1
4391436
d1e3d61
728d2eb
b577c2a
f49c1c6
c362efb
b5bda40
1241c38
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| name: adelphi Python | ||
|
|
||
| on: | ||
| push: | ||
| branches: [ master ] | ||
| pull_request: | ||
| branches: [ master ] | ||
|
|
||
| jobs: | ||
| build: | ||
|
|
||
| runs-on: ubuntu-latest | ||
|
|
||
| steps: | ||
| - uses: actions/checkout@v2 | ||
| - name: Set up Python 3.x | ||
| uses: actions/setup-python@v2 | ||
| with: | ||
| python-version: '3.x' | ||
| - name: Install dependencies | ||
| run: | | ||
| cd python | ||
| pip install ./adelphi | ||
| cd adelphi | ||
| pip install -r ./test-requirements.txt | ||
| - name: Execute tests | ||
| run: | | ||
| cd python/adelphi | ||
| test-adelphi | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,7 +15,6 @@ | |
|
|
||
| # Functions and constants related to the anonymization process | ||
| from adelphi.store import get_standard_columns_from_table_metadata | ||
| import re | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Randomly discovered unused import, presumably a holdover from previous changes |
||
|
|
||
| # default prefixes for the anonymized names | ||
| KEYSPACE_PREFIX = "ks" | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,7 +22,6 @@ | |
|
|
||
| from github import Github | ||
|
|
||
| logging.basicConfig(level=logging.INFO) | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This (and store.py below) should definitely not be setting up their own logging... they should be leveraging logs that are configured somewhere else. |
||
| log = logging.getLogger('adelphi') | ||
|
|
||
| # We're assuming any storage repo will be created after the conversion to "main" | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,7 +27,8 @@ | |
| from cassandra.cluster import Cluster, ExecutionProfile, EXEC_PROFILE_DEFAULT, default_lbp_factory | ||
| from cassandra.auth import PlainTextAuthProvider | ||
|
|
||
| logging.basicConfig(level=logging.INFO) | ||
| from tenacity import retry | ||
|
|
||
| log = logging.getLogger('adelphi') | ||
|
|
||
| system_keyspaces = set(["system", | ||
|
|
@@ -39,20 +40,26 @@ | |
| "system_views"]) | ||
|
|
||
| def build_auth_provider(username = None,password = None): | ||
| # instantiate auth provider if credentials have been provided | ||
| auth_provider = None | ||
| if username is not None and password is not None: | ||
| auth_provider = PlainTextAuthProvider(username=username, password=password) | ||
| return auth_provider | ||
| """Instantiate auth provider if credentials have been provided""" | ||
| if username is None or password is None: | ||
| return None | ||
| return PlainTextAuthProvider(username=username, password=password) | ||
|
|
||
|
|
||
| @retry | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Annotated to make this a tenacity-aware function but we don't provide global configs here since those are largely context-specific. Individual callers can provide args appropriate for their specific use; see the example in test-adelphi to see what this looks like. |
||
| def with_cluster(cluster_fn, hosts, port, username = None, password = None): | ||
| ep = ExecutionProfile(load_balancing_policy=default_lbp_factory()) | ||
| cluster = Cluster(hosts, port=port, auth_provider=build_auth_provider(username,password), execution_profiles={EXEC_PROFILE_DEFAULT: ep}) | ||
| cluster.connect() | ||
| rv = cluster_fn(cluster) | ||
| cluster.shutdown() | ||
| return rv | ||
| try: | ||
| cluster.connect() | ||
| return cluster_fn(cluster) | ||
| finally: | ||
| cluster.shutdown() | ||
|
|
||
|
|
||
| @retry | ||
| def with_local_cluster(cluster_fn): | ||
| return with_cluster(cluster_fn, ["127.0.0.1"], port=9042) | ||
|
|
||
|
|
||
| def build_keyspace_objects(keyspaces, metadata): | ||
|
|
@@ -71,9 +78,7 @@ def partition(pred, iterable): | |
|
|
||
|
|
||
| def get_standard_columns_from_table_metadata(table_metadata): | ||
| """ | ||
| Return the standard columns and ensure to exclude pk and ck ones. | ||
| """ | ||
| """Return the standard columns and ensure to exclude pk and ck ones""" | ||
| partition_column_names = [c.name for c in table_metadata.partition_key] | ||
| clustering_column_names = [c.name for c in table_metadata.clustering_key] | ||
| standard_columns = [] | ||
|
|
@@ -96,3 +101,28 @@ def set_replication_factor(selected_keyspaces, factor): | |
| log.debug("Replication for keyspace " + ks.name+ ": " + str(ks.replication_strategy)) | ||
| strategy = ks.replication_strategy | ||
| strategy.replication_factor_info = factor | ||
|
|
||
|
|
||
| def create_schema(session, schemaPath): | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we move this to the test module?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We probably could, but I'd argue this function belongs in adelphi.store just as much as with_cluster() or with_local_cluster(). It seems like the bigger problem here is that it's poorly named. This function exists to execute a collection of semicolon-terminated CQL statements contained in a file against a CQL instance somewhere. We use it in the tests for setting up schemas but you can imagine any number of usages other than that within Adelphi. I think the better answer would be to rename it to execute_cql() or something like that. wdyt? |
||
| """Read schema CQL document and apply CQL commands to cluster""" | ||
| log.info("Creating schema on Cassandra cluster from file {}".format(schemaPath)) | ||
| with open(schemaPath) as schema: | ||
| buff = "" | ||
| for line in schema: | ||
| realLine = line.strip() | ||
| if len(realLine) == 0: | ||
| log.debug("Skipping empty statement") | ||
| continue | ||
| if realLine.startswith("//") or realLine.startswith("--"): | ||
| log.debug("Skipping commented statement") | ||
| continue | ||
| buff += (" " if len(buff) > 0 else "") | ||
| buff += realLine | ||
| if buff.endswith(';'): | ||
| log.debug("Executing statement {}".format(buff)) | ||
| try: | ||
| session.execute(buff) | ||
| except Exception as exc: | ||
| log.error("Exception executing statement: {}".format(buff), exc_info=exc) | ||
| finally: | ||
| buff = "" | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,8 +10,10 @@ | |
| # suite, which in turn should allow us to write simpler tests. | ||
| import configparser | ||
| import os | ||
| import re | ||
| import sys | ||
|
|
||
| from tests.util.cassandra_util import connectToLocalCassandra | ||
| from adelphi.store import with_local_cluster | ||
|
|
||
| import click | ||
| import docker | ||
|
|
@@ -36,13 +38,40 @@ def runCassandraContainer(client, version): | |
| def writeToxIni(version): | ||
| config = configparser.ConfigParser() | ||
| config["tox"] = { "envlist": "py2, py3" } | ||
| envs = {"CASSANDRA_VERSION": version} | ||
| config["testenv"] = {"deps": TOX_DEPENDENCIES, \ | ||
| "commands": "pytest {posargs}", \ | ||
| "setenv": "CASSANDRA_VERSION = {}".format(version)} | ||
| with open(TOX_CONFIG, 'w') as configfile: | ||
| config.write(configfile) | ||
|
|
||
|
|
||
| def buildVersionMap(): | ||
| assert sorted(DEFAULT_CASSANDRA_VERSIONS) | ||
| rv = {v:v for v in DEFAULT_CASSANDRA_VERSIONS} | ||
| majorMinorPattern = re.compile(r"(\d\.\d).*") | ||
| for v in DEFAULT_CASSANDRA_VERSIONS: | ||
| majorMinorMatch = majorMinorPattern.match(v) | ||
| if majorMinorMatch: | ||
| majorMinor = majorMinorMatch.group(1) | ||
| rv[majorMinor] = v | ||
| # The reason we needed to asset that our input was sorted; we want the last | ||
| # major version entry we discover in the list to map to the major version. | ||
| # So "2" => "2.2.whatever" rather than "2.1.whatever" | ||
| rv[majorMinor.split('.')[0]] = v | ||
| return rv | ||
|
|
||
|
|
||
| def resolveCassandraVersions(cassandra_versions): | ||
| if not cassandra_versions: | ||
| return DEFAULT_CASSANDRA_VERSIONS | ||
| versionMap = buildVersionMap() | ||
| computedVersions = [x for x in [versionMap.get(v) for v in cassandra_versions] if x is not None] | ||
| if not computedVersions: | ||
| print("Could not compute valid Cassandra versions based on args, using defaults") | ||
| return DEFAULT_CASSANDRA_VERSIONS | ||
| return computedVersions | ||
|
|
||
|
|
||
| @click.command() | ||
| @click.option('--cassandra', '-c', multiple=True, type=str) | ||
| @click.option('--python', '-p', multiple=True, type=click.Choice(["py2","py3"], case_sensitive = False)) | ||
|
|
@@ -55,15 +84,21 @@ def runtests(cassandra, python, pytest): | |
| tox_args.append(pytest) | ||
| print("Full tox args: {}".format(tox_args)) | ||
|
|
||
| cassandra_versions = cassandra or DEFAULT_CASSANDRA_VERSIONS | ||
| cassandra_versions = resolveCassandraVersions(cassandra) | ||
| print("Cassandra versions to test: {}".format(','.join(cassandra_versions))) | ||
| for version in cassandra_versions: | ||
| exitCodes = [] | ||
| for version in resolveCassandraVersions(cassandra_versions): | ||
|
|
||
| print("Running test suite for Cassandra version {}".format(version)) | ||
| container = runCassandraContainer(client, version) | ||
|
|
||
| print("Validating connection to local Cassandra") | ||
| connectToLocalCassandra() | ||
| def validationFn(cluster): | ||
| session = cluster.connect() | ||
| rs = session.execute("select * from system.local") | ||
| print("Connected to Cassandra cluster, first row of system.local: {}".format(rs.one())) | ||
| return (cluster, session) | ||
| with_local_cluster.retry_with(stop=stop_after_attempt(5), wait=wait_fixed(3))(validationFn) | ||
|
|
||
| try: | ||
| if os.path.exists(TOX_CONFIG): | ||
|
|
@@ -74,13 +109,13 @@ def runtests(cassandra, python, pytest): | |
| # exiting all the things | ||
| try: | ||
| tox.cmdline(tox_args) | ||
| except SystemExit: | ||
| pass | ||
| except SystemExit as exc: | ||
| exitCodes.append(exc.code) | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The exit code that would be returned by tox lives in SystemExit.code. We need to trap this to determine how many of our tox "invocations" resulted in successful tests. |
||
| except Exception as exc: | ||
| print("Exception running tests for Cassandra version {}".format(version), exc) | ||
| finally: | ||
| container.stop() | ||
|
|
||
| sys.exit(sum(1 for x in exitCodes if x != 0)) | ||
|
|
||
| if __name__ == '__main__': | ||
| runtests(obj={}) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,2 @@ | ||
| click ~= 7.1 | ||
| cassandra-driver ~= 3.24 | ||
| docker ~= 4.4 | ||
| tenacity ~= 7.0 | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now that test-adelphi is a full-fledged part of the Python package (see setup.py change above) there isn't any need to manage it's dependencies via test requirements. This change was one of the significant arguments for making test-adelphi part of the package. |
||
| tox ~= 3.22 | ||
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need to specify the branch name?
We might change the name to trunk to match other repos and we'd have to change it here too.
Also, having it hardcoded won't it make it run less frequently, since all PRs are against master right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly I'm not sure if this field can be empty or not. Most of the samples I found explicitly specified a target branch here but I didn't find any docs one way or the other. That said, I guess I don't view this as a major burden. If we decide to rename master at some point we know that'll be a disruptive change anyway; fixing this usage would just be one thing among many we'd have to address. I also don't agree that all our PRs are off of master at the moment; it's certainly the case that most, even the overwhelming majority, are but there have been a few occasions where I've wanted review of a specific proposal for a feature branch already in-flight... and that leads to branches off of branches. I readily agree it's not common but it does happen.
I'll do some checking to see whether a branch name is required here or not.