58 CI automation of test suite, take two #160
Conversation
…fns are available after installing adelphi Python package. Also cleaned up some unused imports and some excessive logging configs.
…simplify the import of functionality from adelphi packages. Also simplify the dependency specification; test-requirements.txt really should be testing-only stuff now.
…elphi (since this will in turn be required for tox)
…orrectly report failure
…ons which returned a non-zero status code themselves
… tests correctly report failure" This reverts commit 5221b74.
|
|
||
| # Functions and constants related to the anonymization process | ||
| from adelphi.store import get_standard_columns_from_table_metadata | ||
| import re |
There was a problem hiding this comment.
Randomly discovered unused import, presumably a holdover from previous changes
|
|
||
| from github import Github | ||
|
|
||
| logging.basicConfig(level=logging.INFO) |
There was a problem hiding this comment.
This (and store.py below) should definitely not be setting up their own logging... they should be leveraging logs that are configured somewhere else.
| return PlainTextAuthProvider(username=username, password=password) | ||
|
|
||
|
|
||
| @retry |
There was a problem hiding this comment.
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.
| except SystemExit: | ||
| pass | ||
| except SystemExit as exc: | ||
| exitCodes.append(exc.code) |
There was a problem hiding this comment.
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.
| click ~= 7.1 | ||
| cassandra-driver ~= 3.24 | ||
| docker ~= 4.4 | ||
| tenacity ~= 7.0 |
There was a problem hiding this comment.
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.
| push: | ||
| branches: [ master ] | ||
| pull_request: | ||
| branches: [ master ] |
There was a problem hiding this comment.
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.
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.
| strategy.replication_factor_info = factor | ||
|
|
||
|
|
||
| def create_schema(session, schemaPath): |
There was a problem hiding this comment.
Can we move this to the test module?
There was a problem hiding this comment.
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?
No description provided.