diff --git a/apps/cli/utilities/s_code_project_updater/src/s_code_project_updater/commands.py b/apps/cli/utilities/s_code_project_updater/src/s_code_project_updater/commands.py index 4a90f58f6edc5d68723e8ac6c5493a44682d5e5e..1b0be6cad27bdb4003a7bd0c923df4046fd5458b 100644 --- a/apps/cli/utilities/s_code_project_updater/src/s_code_project_updater/commands.py +++ b/apps/cli/utilities/s_code_project_updater/src/s_code_project_updater/commands.py @@ -11,9 +11,10 @@ import sys import warnings from typing import List +from sqlalchemy import exc as sa_exc, asc, desc + from pymygdala import LogHandler, SendNRAOEvent from s_code_project_updater import Telescope - from shared.schema.src.schema import Author, Project, ExecutionBlock, \ create_session from shared.schema.src.schema.pstmodel import Person, UserAuthentication @@ -21,8 +22,6 @@ from shared.support.src.support.capo import get_my_capo_config from shared.support.src.support.logging import LOG_MESSAGE_FORMATTER, \ get_console_logger -from sqlalchemy import exc as sa_exc, asc, desc - from ._version import ___version___ as version from .project_fetcher import ArchiveProjectFetcher @@ -55,9 +54,7 @@ class ScodeProjectUpdater: """ self._make_parser() try: - _LOG.warning('parsing....') self.args = self.parser.parse_args(**kwargs) - _LOG.warning('parsed') except Exception as exc: _LOG.error(f'parser threw {exc}') @@ -76,10 +73,7 @@ class ScodeProjectUpdater: args_dict = self.args.__dict__ - if args_dict['dry']: - self.is_dry = args_dict['dry'] - else: - self.is_dry = False + self.is_dry = args_dict['dry'] if not args_dict['investigators'] \ and not args_dict['title'] and not args_dict['abstract']: @@ -278,18 +272,21 @@ class ScodeProjectUpdater: [_LOG.info(line) for line in output] def get_project_info(self): + output = [] + if self.stored_project is None: + self.stored_project = self.get_stored_project() output.append(f'Title: {self.stored_project.title}') output.append(f'Abstract: {self.stored_project.abstract}') investigator_list = self.get_projects_current_investigators() # we want the PI's pst_person_id followed by the CoIs' pst_person_ids in numeric order - pi = investigator_list[0] - if pi.pst_person_id is not None: + pi_author = investigator_list[0] + if pi_author.pst_person_id is not None: coi_pst_ids = [int(coi.pst_person_id) for coi in investigator_list[1:]] coi_pst_ids = sorted(coi_pst_ids) - author_pst_ids = [int(pi.pst_person_id)] + author_pst_ids = [int(pi_author.pst_person_id)] [author_pst_ids.append(id) for id in coi_pst_ids] authors_to_print = [str(id) for id in author_pst_ids] id_list = ' '.join(authors_to_print) @@ -299,19 +296,30 @@ class ScodeProjectUpdater: def is_fetch_only(self): try: - return self.fetch_only + return self.args.title is None \ + and self.args.abstract is None \ + and self.args.investigators is None except AttributeError: return False def update_project(self) -> Project: ''' - Where the magic happens: update aspects of the project - (including authors) according to the arguments passed in - :return: + The main function responsible for updating the project. + It makes sure the project exists, and, if the user is updating + the investigators, that they have valid PST mappings. If there + aren't errors with those two checks it clears the projects current + archive authors and replaces them with the investigators found + from the PST mapping to users. And, of course, if the title and + abstract are being updated, it adds those to the project. + + :return: Project + ''' + fetcher = ArchiveProjectFetcher(self.args.profile) + project = None try: - self.project = fetcher.fetch_project(self.project_code) + project = fetcher.fetch_project(self.project_code) except AttributeError: self.exit_with_error(f'project code "{self.project_code}" not ' f'found', 3) @@ -319,19 +327,11 @@ class ScodeProjectUpdater: output = fetcher.build_project_info() try: [_LOG.info(line) for line in output] + return project except TypeError: _LOG.error( 'Cannot display project info; is this an ALMA project?') - return self.project - """ - The main function responsible for updating the project. It makes sure the project exists, - and, if the user is updating the investigators, that they have valid PST mappings. If there - aren't errors with those two checks it clears the projects current archive authors and - replaces them with the investigators found from the PST mapping to users. And, of course, - if the title and abstract are being updated, it adds those to the project - :return: None - """ with warnings.catch_warnings(): # Suppress SQLAlchemy warnings warnings.simplefilter("ignore", category=sa_exc.SAWarning) @@ -351,8 +351,8 @@ class ScodeProjectUpdater: proposed_investigators = self.get_pst_users( self.args.investigators) if len(proposed_investigators) == 0 or \ - not len(self.args.investigators) == len( - proposed_investigators): + len(self.args.investigators) \ + != len(proposed_investigators): self.exit_with_error( 'One or more of the investigators you entered was not ' 'found in the PST.', 4) @@ -364,16 +364,16 @@ class ScodeProjectUpdater: if self.args.abstract: self.stored_project.abstract = self.args.abstract - if not self.args.dry: - if not self.is_fetch_only(): - self.archive_session.commit() - _LOG.info('Changes committed') - elif not self.is_fetch_only(): + if not self.is_dry: + self.archive_session.commit() + _LOG.info('Changes committed') + else: + self.archive_session.rollback() _LOG.info( 'Successful dry run; this would have updated the project') self.print_project() - return self.stored_project + return self.get_stored_project() def is_alma(self): ''' is this an alma project? ''' @@ -455,6 +455,7 @@ class ArchiveProject: options.append('-I') options.append('--investigators') self.options = options + self.profile = os.environ['CAPO_PROFILE'] def make_args(self, is_dry): args = [] @@ -464,7 +465,6 @@ class ArchiveProject: args.append('-C') args.append(self.project_code) args.append('-P') - self.profile = os.environ['CAPO_PROFILE'] args.append(self.profile) args.append('-T') args.append(self.title) @@ -482,16 +482,17 @@ class ArchiveProject: return arg in self.options def add_parameter(self, new_project, key, value): - if '-C' == key or '--project' == key: + if key in ('-C', '--project'): new_project.project_code = value - elif '-T' == key or '--title' == key: + elif key in ('-T', '--title'): new_project.title = value - elif '-A' == key or '--abstract' == key: + elif key in ('-A', '--abstract'): new_project.abstract = value - elif '-P' == key or '--profile' == key: + elif key in ('-P', '--profile'): self.profile = value def add_investigators(self, new_project, args, start_position): + ''' Add specified investigators to project ''' value = args[start_position] while value not in self.options: @@ -532,12 +533,14 @@ class UpdateArgParser: return parser def parse_args(self, **kwargs): + ''' Try to parse command-line arguments, and fail informatively + if there are errors + ''' return self.parser.parse_args(kwargs) class UpdateException(Exception): ''' throw this if there is trouble during the update ''' - pass def main(**kwargs): diff --git a/apps/cli/utilities/s_code_project_updater/test/test_updater.py b/apps/cli/utilities/s_code_project_updater/test/test_updater.py index bc8eb6c466e8969d5acdaab89896844887e5d22d..6f161f685b73d42f22c9e5d96223b99e15a241de 100755 --- a/apps/cli/utilities/s_code_project_updater/test/test_updater.py +++ b/apps/cli/utilities/s_code_project_updater/test/test_updater.py @@ -4,13 +4,13 @@ import subprocess import unittest import warnings -import pytest from sqlalchemy import exc as sa_exc -from s_code_project_updater.commands import ScodeProjectUpdater, UpdateException +import pytest +from s_code_project_updater.commands import ScodeProjectUpdater from shared.schema.src.schema import create_session, Project -from shared.schema.src.schema.pstmodel import Session from shared.support.src.support.logging import get_console_logger + from .test_projects import \ ScodeTestProject, ScienceTestProject, AlmaTestProject, get_author_pst_ids @@ -19,6 +19,7 @@ _UPDATE_COMMAND = 'update_sproj' PROFILE = 'local' class UpdaterTestCase(unittest.TestCase): + ''' Exercises ScodeProjectUpdater ''' @classmethod def setUpClass(cls) -> None: @@ -36,8 +37,6 @@ class UpdaterTestCase(unittest.TestCase): def test_dry_run_does_not_update(self): fake_project = ScodeTestProject().project project_code = fake_project.project_code - session = create_session('SDM') - return_code = None try: new_title = 'this is the new title' self.assertNotEqual(fake_project.title, new_title, @@ -49,33 +48,25 @@ class UpdaterTestCase(unittest.TestCase): '-T', new_title, '--dry' ] - try: - return_code = CommandLineUpdaterLauncher(args).run() - except Exception as exc: - text = self.return_values[return_code] if return_code else '' - pytest.fail(f'{exc} {text}') - - if not return_code: - updated = self.get_project_from_db(session, project_code) - # nothing should have been updated - self.assertEqual(fake_project.title, updated.title, - f'expecting same title, but before is ' - f'{fake_project.title} and after is {updated.title}') - self.assertEqual(fake_project.abstract, updated.abstract, - f'expecting same abstract, but before is ' - f'{fake_project.abstract} and updated is {updated.abstract}') - self.assertEqual(len(fake_project.authors), - len(updated.authors), - f'expecting same number of authors, ' - f'but before has {len(fake_project.authors)} ' - f'and after has {len(updated.authors)}') - else: - pytest.fail(f'unexpected failure; return code ={return_code}') - + updated = ScodeProjectUpdater(args=args).update_project() + # nothing should have been updated + self.assertEqual(fake_project.title, updated.title, + f'expecting same title, but before is ' + f'{fake_project.title} and after is {updated.title}') + self.assertEqual(fake_project.abstract, updated.abstract, + f'expecting same abstract, but before is ' + f'{fake_project.abstract} and updated is {updated.abstract}') + self.assertEqual(len(fake_project.authors), + len(updated.authors), + f'expecting same number of authors, ' + f'but before has {len(fake_project.authors)} ' + f'and after has {len(updated.authors)}') + except SystemExit as exc: + pytest.fail(f'unexpected failure with return code {exc.code}') + raise except Exception as exc: pytest.fail(f'{project_code}: {exc}') - finally: - session.close() + raise def test_project_code_only_fetches(self): fake_project = ScodeTestProject().project @@ -84,85 +75,68 @@ class UpdaterTestCase(unittest.TestCase): '-C', project_code, '-P', PROFILE, ] - return_code = None + + updated = None try: - return_code = CommandLineUpdaterLauncher(args).run() - if not return_code: - session = create_session('SDM') - try: - updated = self.get_project_from_db(session, project_code) - self.assertEqual(fake_project.title, updated.title, - f'expecting same title, but before is ' - f'{fake_project.title} and after is {updated.title}') - self.assertEqual(fake_project.abstract, updated.abstract, - f'expecting same abstract, but before is ' - f'{fake_project.abstract} and updated is {updated.abstract}') - self.assertEqual(len(fake_project.authors), - len(updated.authors), - f'expecting same number of authors, ' - f'but before has {len(fake_project.authors)} ' - f'and after has {len(updated.authors)}') - count = 0 - for orig_author in fake_project.authors: - for author in updated.authors: - if author.username == orig_author.username: - count += 1 - break - self.assertEqual(len(fake_project.authors), count, - 'before and after projects should have ' - 'same authors') - finally: - session.close() - except Exception as exc: - text = self.return_values[return_code] if return_code else '' - pytest.fail(f'{exc} {text}') + updated = ScodeProjectUpdater(args=args).update_project() + except SystemExit as exc: + pytest.fail(f'unexpected failure with return code {exc.code}') + raise + + self.assertIsNotNone(updated, 'we should have gotten a project back') + + self.assertEqual(fake_project.title, updated.title, + f'expecting same title, but before is ' + f'{fake_project.title} and after is {updated.title}') + self.assertEqual(fake_project.abstract, updated.abstract, + f'expecting same abstract, but before is ' + f'{fake_project.abstract} and updated is {updated.abstract}') + self.assertEqual(len(fake_project.authors), + len(updated.authors), + f'expecting same number of authors, ' + f'but before has {len(fake_project.authors)} ' + f'and after has {len(updated.authors)}') + count = 0 + for orig_author in fake_project.authors: + for author in updated.authors: + if author.username == orig_author.username: + count += 1 + break + self.assertEqual(len(fake_project.authors), count, + 'before and after projects should have ' + 'same authors') def test_updates_abstract_only(self): fake_project = ScodeTestProject().project project_code = fake_project.project_code - session = create_session('SDM') new_abstract = "Well, here's another nice mess you've gotten us into, Ollie" self.assertNotEqual(fake_project.abstract, new_abstract, - f'expecting new abstract {new_abstract} but got {fake_project.abstract}') + f'expecting new abstract {new_abstract} ' + f'but got {fake_project.abstract}') args = [ '-C', project_code, '-P', PROFILE, '-A', new_abstract, ] - return_code = None try: - try: - return_code = CommandLineUpdaterLauncher(args).run() - except subprocess.TimeoutExpired as exp: - raise UpdateException(exp) - except Exception as exc: - text = self.return_values[return_code] if return_code else '' - pytest.fail(f'{exc} {text}') - - if not return_code: - updated = self.get_project_from_db(session, project_code) - # only abstract should have been updated; - # all else should be same - self.assertEqual(fake_project.title, updated.title, - f'expecting same title, but before is ' - f'{fake_project.title} and after is {updated.title}') - self.assertEqual(new_abstract, updated.abstract, - f'expecting same abstract, but before is ' - f'{fake_project.abstract} and updated is {updated.abstract}') - self.assertEqual(len(fake_project.authors), - len(updated.authors)) - else: - raise UpdateException() - - except Exception as exc: - pytest.fail(f'{project_code}: {exc}') - finally: - session.close() + updated = ScodeProjectUpdater(args=args).update_project() + # only abstract should have been updated; + # all else should be same + self.assertEqual(fake_project.title, updated.title, + f'expecting same title, but before is ' + f'{fake_project.title} and after is {updated.title}') + self.assertEqual(new_abstract, updated.abstract, + f'expecting same abstract, but before is ' + f'{fake_project.abstract} and updated is {updated.abstract}') + self.assertEqual(len(fake_project.authors), + len(updated.authors)) + except SystemExit as exc: + pytest.fail(f'unexpected failure; return code = {exc.code}') + raise def test_updates_abstract_and_title(self): fake_project = ScodeTestProject().project project_code = fake_project.project_code - session = create_session('SDM') new_abstract = "I think you ought to know I'm feeling very depressed" new_title = 'A Survey of the Mattresses of Sqornshellous Zeta' self.assertNotEqual(fake_project.abstract, new_abstract, @@ -177,34 +151,18 @@ class UpdaterTestCase(unittest.TestCase): '-A', new_abstract, '-T', new_title, ] - return_code = None try: - try: - return_code = CommandLineUpdaterLauncher(args).run() - except subprocess.TimeoutExpired as exp: - raise UpdateException(exp) - except Exception as exc: - text = self.return_values[return_code] if return_code else '' - pytest.fail(f'{exc} {text}') - - if not return_code: - updated = self.get_project_from_db(session, project_code) - # abstract and title should have been updated; - # all else should be same - self.assertEqual(new_title, updated.title, - 'title should not have changed') - self.assertEqual(new_abstract, updated.abstract, - 'abstract should not have changed') - self.assertEqual(len(fake_project.authors), - len(updated.authors), - 'authors should not have changed') - else: - raise UpdateException() - - except Exception as exc: - pytest.fail(f'{project_code}: {exc}') - finally: - session.close() + updated = ScodeProjectUpdater(args=args).update_project() + self.assertEqual(new_title, updated.title, + 'title should not have changed') + self.assertEqual(new_abstract, updated.abstract, + 'abstract should not have changed') + self.assertEqual(len(fake_project.authors), + len(updated.authors), + 'authors should not have changed') + except SystemExit as exc: + pytest.fail(f'unexpected failure; exit code = {exc.code}') + raise def test_adds_new_abstract_deletes_author(self): fake_project = ScodeTestProject().project @@ -231,44 +189,36 @@ class UpdaterTestCase(unittest.TestCase): '-A', new_abstract, '-I', ] - for id in get_author_pst_ids(new_project): - args.append(str(id)) + for author_id in get_author_pst_ids(new_project): + args.append(str(author_id)) - session = create_session('SDM') + updated = None try: - try: - ScodeProjectUpdater(args=args).update_project() - except Exception as exc: - pytest.fail(f'{exc}') - - updated = self.get_project_from_db(session, project_code) - # last author should have been removed and the abstract changed; - # title should remain same - self.assertNotEqual(fake_project.abstract, updated.abstract, - 'abstract should have changed') - self.assertEqual(fake_project.title, updated.title, - 'title should not have changed') - expected = len(original_authors) - 1 - actual = len(updated.authors) - self.assertEqual(expected, actual, - 'one author should have been removed') - authors_updated = last_author in updated.authors - self.assertFalse(authors_updated, 'THIS IS THE MESSAGE') - count = 0 - for orig_author in original_authors[:3]: - for new_author in updated.authors: - if new_author.username == orig_author.username: - count += 1 - break - self.assertEqual(len(new_authors), count, - f'expected {len(new_authors)} authors in ' - f'updated project; there were {count}') - - except Exception as exc: - pytest.fail(f'{project_code}: {exc}') - - finally: - session.close() + updated = ScodeProjectUpdater(args=args).update_project() + self.assertIsNotNone(updated, 'project should have been returned') + except SystemExit as exc: + pytest.fail(f'unexpected failure; return code = {exc.code}') + raise + + self.assertNotEqual(fake_project.abstract, updated.abstract, + 'abstract should have changed') + self.assertEqual(fake_project.title, updated.title, + 'title should not have changed') + expected = len(original_authors) - 1 + actual = len(updated.authors) + self.assertEqual(expected, actual, + 'one author should have been removed') + authors_updated = last_author in updated.authors + self.assertFalse(authors_updated, 'THIS IS THE MESSAGE') + count = 0 + for orig_author in original_authors[:3]: + for new_author in updated.authors: + if new_author.username == orig_author.username: + count += 1 + break + self.assertEqual(len(new_authors), count, + f'expected {len(new_authors)} authors in ' + f'updated project; there were {count}') def test_output_is_as_expected(self): fake_project = ScodeTestProject().project @@ -277,30 +227,28 @@ class UpdaterTestCase(unittest.TestCase): '-C', project_code, '-P', PROFILE, ] - - runner = CommandLineUpdaterLauncher(args) - return_code = runner.run() - if return_code: - text = self.return_values[return_code] - pytest.fail(text) - - stdout = runner.stdout - self.assertIsNotNone(stdout, 'program output is expected') - self.assertTrue('Title: ' + fake_project.title in stdout, + updater = ScodeProjectUpdater(args=args) + updater.update_project() + output = updater.get_project_info() + self.assertIsNotNone(output, 'program output is expected') + self.assertTrue('Title: ' + fake_project.title in output, 'title should be in output') - self.assertTrue('Abstract: ' + fake_project.abstract in stdout, + self.assertTrue('Abstract: ' + fake_project.abstract in output, 'abstract should be in output') pst_ids = [str(id) for id in get_author_pst_ids(fake_project)] pst_id_str = ' '.join(pst_ids) - self.assertTrue('Authors: ' + pst_id_str in stdout, + self.assertTrue('Authors: ' + pst_id_str in output, f'output should have PST IDs {pst_ids}') def test_copes_with_single_pi(self): project = ScodeTestProject().project args = ['-P', PROFILE, '-C', project.project_code, '-I', '4686'] - return_code = CommandLineUpdaterLauncher(args=args).run() - self.assertEqual(0, return_code, - 'update to single author should succeed') + try: + updated = ScodeProjectUpdater(args=args).update_project() + self.assertEqual(1, len(updated.authors)) + except SystemExit as ex: + pytest.fail(f'update failed with exit code {ex.code}') + raise def test_alma_project_is_rejected(self): project_code = '2018.A.00062.S' @@ -312,7 +260,26 @@ class UpdaterTestCase(unittest.TestCase): ScodeProjectUpdater(args=args).update_project() self.assertEqual(2, exc.code, 'ALMA project should be rejected') - def test_errors_return_expected_codes(self): + def test_update_failure_returns_expected_code(self): + result = FailingUpdater().update_project() + self.assertIsInstance(result, SystemExit) + self.assertEqual(5, result.code, + 'expecting return code 5 for update failure') + + """ The following test should be moved to another test case, + where we'll use a bash script, via subprocess.call(), to create an + appropriate env and execute pytest. + """ + @pytest.mark.skip('pytest passes only in IJ; ' + 'fails when run from command line' + 'due to import errors') + def test_command_line_returns_expected_codes(self): + ''' We simulate execution from the command line + and confirm that errors result in the appropriate + return codes. + + ''' + # minimum required arguments -- profile & project -- omitted return_code = CommandLineUpdaterLauncher([]).run() self.assertEqual(return_code, 2, @@ -320,12 +287,6 @@ class UpdaterTestCase(unittest.TestCase): project_code = ScodeTestProject().project.project_code - # update failure - result = FailingUpdater().update_project() - self.assertIsInstance(result, SystemExit) - self.assertEqual(5, result.code, - 'expecting return code 5 for update failure') - # profile not specified args = ['-C', project_code,] return_code = CommandLineUpdaterLauncher(args).run() @@ -377,6 +338,7 @@ class UpdaterTestCase(unittest.TestCase): ### UTILITIES ### def initialize_test_data(self): + ''' Insert test data into archive database for use in tests ''' session = create_session('SDM') num_commits = num_found = 0 try: @@ -408,10 +370,12 @@ class UpdaterTestCase(unittest.TestCase): f'added and committed') except Exception as exc: pytest.fail(f'{exc}') + raise finally: session.close() def remove_test_data(self): + ''' Get rid of the test data we inserted. ''' session = create_session('SDM') try: with warnings.catch_warnings(): @@ -430,20 +394,18 @@ class UpdaterTestCase(unittest.TestCase): if existing is not None: session.delete(existing) session.commit() + + # confirm removal + found = session.query(Project) \ + .filter(Project.project_code.like('%_TEST_PROJECT')) \ + .first() + if found is not None: + pytest.fail('test projects were not removed') except Exception as exc: pytest.fail(f'{exc}') finally: session.close() - def get_project_from_db(self, session: Session, project_code: str): - with warnings.catch_warnings(): - # Suppress SQLAlchemy warnings - warnings.simplefilter("ignore", category=sa_exc.SAWarning) - - return session.query(Project) \ - .filter(Project.project_code == project_code) \ - .first() - class FailingUpdaterHelper: ''' for use in testing update failure ''' @@ -460,7 +422,11 @@ class FailingUpdater: return SystemExit(5) class CommandLineUpdaterLauncher: - ''' Simulates execution of script from command line ''' + ''' Simulates execution of script from command line. + This works when tests are run from within iJ + but not when pytest is execuated at the command line. + ''' + def __init__(self, args: list): self.args = [_UPDATE_COMMAND] for arg in args: @@ -468,23 +434,19 @@ class CommandLineUpdaterLauncher: _LOG.info(f'{self.args}') def run(self): - ''' launch updater from command line + ''' launch updater in a subprocess @:returns directory listing ''' - # TODO: This works fine if pytest is executed from within IJ - # but fails if it's run from the command line: can't find schema imports in - # commands.py. Do it in a bash script with subprocess.call()? See - # deploy.sh and datafetcher Dockerfile args = self.args try: proc = subprocess.run(args, - stdout=subprocess.PIPE, - stderr=subprocess.STDOUT, - timeout=60, - check=False, - bufsize=1, - universal_newlines=True) + stdout=subprocess.PIPE, + stderr=subprocess.STDOUT, + timeout=60, + check=False, + bufsize=1, + universal_newlines=True) self.stdout = proc.stdout if proc.returncode: print(f'{self.stdout}') @@ -494,7 +456,7 @@ class CommandLineUpdaterLauncher: return exc.returncode def build_updater_return_values(): - ''' these are the same return codes and messages in the updater's "usage" string ''' + ''' return codes and messages in the updater's "usage" string ''' return { 1: 'error with capo configuration', 2: 'error with input parameters',