From bb60ac96d4db6689f1129c81febf26285bc61899 Mon Sep 17 00:00:00 2001 From: Eu Pin Tien Date: Tue, 6 May 2025 09:31:34 +0100 Subject: [PATCH 01/47] Passed ISPyB credentials in to 'url()' function to remove the need for an ISPYB_CREDENTIALS environment variable --- src/murfey/server/ispyb.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/murfey/server/ispyb.py b/src/murfey/server/ispyb.py index c44f19ccf..8a516ee2f 100644 --- a/src/murfey/server/ispyb.py +++ b/src/murfey/server/ispyb.py @@ -39,9 +39,14 @@ try: Session = sqlalchemy.orm.sessionmaker( - bind=sqlalchemy.create_engine(url(), connect_args={"use_pure": True}) + bind=sqlalchemy.create_engine( + url(credentials=security_config.ispyb_credentials), + connect_args={"use_pure": True}, + ) ) + log.info("Loaded ISPyB database session") except AttributeError: + log.error("Error loading ISPyB session", exc_info=True) Session = lambda: None @@ -67,6 +72,8 @@ def __init__(self, transport_type: Literal["PikaTransport"]): if security_config.ispyb_credentials else None ) + if self.ispyb is not None: + print("Loaded ISPyB databse") self._connection_callback: Callable | None = None def reconnect(self): @@ -632,6 +639,7 @@ def get_all_ongoing_visits( microscope: str, db: sqlalchemy.orm.Session | None ) -> list[Visit]: if db is None: + print("No database found") return [] query = ( db.query(BLSession) From b48d654c537bd5763a0aca60aa6922e764a86481 Mon Sep 17 00:00:00 2001 From: Eu Pin Tien Date: Tue, 6 May 2025 09:36:01 +0100 Subject: [PATCH 02/47] Added debug log to API endpoint for loading ongoing visits --- src/murfey/server/api/__init__.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/murfey/server/api/__init__.py b/src/murfey/server/api/__init__.py index f63ca27aa..a833118da 100644 --- a/src/murfey/server/api/__init__.py +++ b/src/murfey/server/api/__init__.py @@ -892,6 +892,9 @@ def _add_tilt(): @router.get("/instruments/{instrument_name}/visits_raw", response_model=List[Visit]) def get_current_visits(instrument_name: str, db=murfey.server.ispyb.DB): + log.debug( + f"Received request to look up ongoing visits for {sanitise(instrument_name)}" + ) return murfey.server.ispyb.get_all_ongoing_visits(instrument_name, db) From b68a06212f1c62f5da3832a6f78f5b1c548e9476 Mon Sep 17 00:00:00 2001 From: Eu Pin Tien Date: Tue, 6 May 2025 14:58:47 +0100 Subject: [PATCH 03/47] Simplified imports of classes and functions in 'murfey.server.ispyb' --- src/murfey/server/ispyb.py | 55 +++++++++++++++++--------------------- 1 file changed, 25 insertions(+), 30 deletions(-) diff --git a/src/murfey/server/ispyb.py b/src/murfey/server/ispyb.py index 8a516ee2f..e6aab1065 100644 --- a/src/murfey/server/ispyb.py +++ b/src/murfey/server/ispyb.py @@ -5,9 +5,6 @@ from typing import Callable, Generator, List, Literal, Optional import ispyb - -# import ispyb.sqlalchemy -import sqlalchemy.orm import workflows.transport from fastapi import Depends from ispyb.sqlalchemy import ( @@ -29,6 +26,8 @@ ZcZocaloBuffer, url, ) +from sqlalchemy import create_engine +from sqlalchemy.orm import Session, sessionmaker from murfey.util import sanitise from murfey.util.config import get_security_config @@ -38,8 +37,8 @@ security_config = get_security_config() try: - Session = sqlalchemy.orm.sessionmaker( - bind=sqlalchemy.create_engine( + ISPyBSession = sessionmaker( + bind=create_engine( url(credentials=security_config.ispyb_credentials), connect_args={"use_pure": True}, ) @@ -47,7 +46,7 @@ log.info("Loaded ISPyB database session") except AttributeError: log.error("Error loading ISPyB session", exc_info=True) - Session = lambda: None + ISPyBSession = lambda: None def _send_using_new_connection(transport_type: str, queue: str, message: dict) -> None: @@ -94,7 +93,7 @@ def do_insert_data_collection_group( **kwargs, ): try: - with Session() as db: + with ISPyBSession() as db: db.add(record) db.commit() log.info(f"Created DataCollectionGroup {record.dataCollectionGroupId}") @@ -109,7 +108,7 @@ def do_insert_data_collection_group( def do_insert_atlas(self, record: Atlas): try: - with Session() as db: + with ISPyBSession() as db: db.add(record) db.commit() log.info(f"Created Atlas {record.atlasId}") @@ -126,7 +125,7 @@ def do_update_atlas( self, atlas_id: int, atlas_image: str, pixel_size: float, slot: int ): try: - with Session() as db: + with ISPyBSession() as db: atlas = db.query(Atlas).filter(Atlas.atlasId == atlas_id).one() atlas.atlasImage = atlas_image or atlas.atlasImage atlas.pixelSize = pixel_size or atlas.pixelSize @@ -194,7 +193,7 @@ def do_insert_grid_square( pixelSize=grid_square_parameters.pixel_size, ) try: - with Session() as db: + with ISPyBSession() as db: db.add(record) db.commit() log.info(f"Created GridSquare {record.gridSquareId}") @@ -211,7 +210,7 @@ def do_update_grid_square( self, grid_square_id: int, grid_square_parameters: GridSquareParameters ): try: - with Session() as db: + with ISPyBSession() as db: grid_square = ( db.query(GridSquare) .filter(GridSquare.gridSquareId == grid_square_id) @@ -302,7 +301,7 @@ def do_insert_foil_hole( pixelSize=foil_hole_parameters.pixel_size, ) try: - with Session() as db: + with ISPyBSession() as db: db.add(record) db.commit() log.info(f"Created FoilHole {record.foilHoleId}") @@ -322,7 +321,7 @@ def do_update_foil_hole( foil_hole_parameters: FoilHoleParameters, ): try: - with Session() as db: + with ISPyBSession() as db: foil_hole = ( db.query(FoilHole).filter(FoilHole.foilHoleId == foil_hole_id).one() ) @@ -380,7 +379,7 @@ def do_insert_data_collection(self, record: DataCollection, message=None, **kwar else "Created for Murfey" ) try: - with Session() as db: + with ISPyBSession() as db: record.comments = comment db.add(record) db.commit() @@ -396,7 +395,7 @@ def do_insert_data_collection(self, record: DataCollection, message=None, **kwar def do_insert_sample_group(self, record: BLSampleGroup) -> dict: try: - with Session() as db: + with ISPyBSession() as db: db.add(record) db.commit() log.info(f"Created BLSampleGroup {record.blSampleGroupId}") @@ -411,7 +410,7 @@ def do_insert_sample_group(self, record: BLSampleGroup) -> dict: def do_insert_sample(self, record: BLSample, sample_group_id: int) -> dict: try: - with Session() as db: + with ISPyBSession() as db: db.add(record) db.commit() log.info(f"Created BLSample {record.blSampleId}") @@ -434,7 +433,7 @@ def do_insert_sample(self, record: BLSample, sample_group_id: int) -> dict: def do_insert_subsample(self, record: BLSubSample) -> dict: try: - with Session() as db: + with ISPyBSession() as db: db.add(record) db.commit() log.info(f"Created BLSubSample {record.blSubSampleId}") @@ -449,7 +448,7 @@ def do_insert_subsample(self, record: BLSubSample) -> dict: def do_insert_sample_image(self, record: BLSampleImage) -> dict: try: - with Session() as db: + with ISPyBSession() as db: db.add(record) db.commit() log.info(f"Created BLSampleImage {record.blSampleImageId}") @@ -532,7 +531,7 @@ def do_update_processing_status(self, record: AutoProcProgram, **kwargs): return {"success": False, "return_value": None} def do_buffer_lookup(self, app_id: int, uuid: int) -> Optional[int]: - with Session() as db: + with ISPyBSession() as db: buffer_objects = ( db.query(ZcZocaloBuffer) .filter_by(AutoProcProgramID=app_id, UUID=uuid) @@ -543,8 +542,8 @@ def do_buffer_lookup(self, app_id: int, uuid: int) -> Optional[int]: return reference -def _get_session() -> Generator[Optional[sqlalchemy.orm.Session], None, None]: - db = Session() +def _get_session() -> Generator[Optional[Session], None, None]: + db = ISPyBSession() if db is None: yield None return @@ -563,7 +562,7 @@ def get_session_id( proposal_code: str, proposal_number: str, visit_number: str, - db: sqlalchemy.orm.Session | None, + db: Session | None, ) -> int | None: # Log received lookup parameters @@ -596,9 +595,7 @@ def get_session_id( return res -def get_proposal_id( - proposal_code: str, proposal_number: str, db: sqlalchemy.orm.Session -) -> int: +def get_proposal_id(proposal_code: str, proposal_number: str, db: Session) -> int: query = ( db.query(Proposal) .filter( @@ -610,7 +607,7 @@ def get_proposal_id( return query[0].proposalId -def get_sub_samples_from_visit(visit: str, db: sqlalchemy.orm.Session) -> List[Sample]: +def get_sub_samples_from_visit(visit: str, db: Session) -> List[Sample]: proposal_id = get_proposal_id(visit[:2], visit.split("-")[0][2:], db) samples = ( db.query(BLSampleGroup, BLSampleGroupHasBLSample, BLSample, BLSubSample) @@ -635,9 +632,7 @@ def get_sub_samples_from_visit(visit: str, db: sqlalchemy.orm.Session) -> List[S return res -def get_all_ongoing_visits( - microscope: str, db: sqlalchemy.orm.Session | None -) -> list[Visit]: +def get_all_ongoing_visits(microscope: str, db: Session | None) -> list[Visit]: if db is None: print("No database found") return [] @@ -676,7 +671,7 @@ def get_all_ongoing_visits( def get_data_collection_group_ids(session_id): query = ( - Session() + ISPyBSession() .query(DataCollectionGroup) .filter( DataCollectionGroup.sessionId == session_id, From 03897de669c766725d60bd1376740ea38ccba3dd Mon Sep 17 00:00:00 2001 From: Eu Pin Tien Date: Tue, 6 May 2025 15:00:48 +0100 Subject: [PATCH 04/47] Renamed URL and Engine for Murfey DB to make it more self-evident --- tests/__init__.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/__init__.py b/tests/__init__.py index 92085ab06..1f1407cb1 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -2,9 +2,9 @@ from sqlmodel import create_engine -url = ( +murfey_db_url = ( f"postgresql+psycopg2://{os.environ['POSTGRES_USER']}:{os.environ['POSTGRES_PASSWORD']}" f"@{os.environ['POSTGRES_HOST']}:{os.environ['POSTGRES_PORT']}/{os.environ['POSTGRES_DB']}" ) -engine = create_engine(url) +murfey_db_engine = create_engine(murfey_db_url) From 97dc116821bd2866b8cdcf656522df3b76f97a5e Mon Sep 17 00:00:00 2001 From: Eu Pin Tien Date: Tue, 6 May 2025 15:02:19 +0100 Subject: [PATCH 05/47] Added fixture for ISPyB credentials file; modified security configuration file fixture to return file path as part of the fixture --- tests/conftest.py | 43 ++++++++++++++++++++++++++++++++----------- 1 file changed, 32 insertions(+), 11 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index 9176d86a1..7413a9158 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,23 +1,22 @@ import json from configparser import ConfigParser +from pathlib import Path import pytest from sqlmodel import Session from murfey.util.db import Session as MurfeySession from murfey.util.db import clear, setup -from tests import engine, url - -mock_security_config_name = "security_config.yaml" +from tests import murfey_db_engine, murfey_db_url @pytest.fixture def start_postgres(): - clear(url) - setup(url) + clear(murfey_db_url) + setup(murfey_db_url) murfey_session = MurfeySession(id=2, name="cm12345-6") - with Session(engine) as murfey_db: + with Session(murfey_db_engine) as murfey_db: murfey_db.add(murfey_session) murfey_db.commit() @@ -37,15 +36,37 @@ def mock_client_configuration() -> ConfigParser: @pytest.fixture() -def mock_security_configuration(tmp_path): - config_file = tmp_path / mock_security_config_name +def mock_ispyb_credentials(tmp_path: Path): + creds_file = tmp_path / "ispyb_creds.cfg" + ispyb_config = ConfigParser() + # Use values from the GitHub workflow ISPyB config file + ispyb_config["ispyb_sqlalchemy"] = { + "username": "ispyb_api_sqlalchemy", + "password": "password_5678", + "host": "localhost", + "port": "3306", + "database": "ispybtest", + } + with open(creds_file, "w") as file: + ispyb_config.write(file) + return creds_file + + +@pytest.fixture() +def mock_security_configuration( + tmp_path: Path, + mock_ispyb_credentials: Path, +): + config_file = tmp_path / "security_config.yaml" security_config = { + "murfey_db_credentials": "/path/to/murfey_db_credentials", + "crypto_key": "crypto_key", "auth_key": "auth_key", "auth_algorithm": "auth_algorithm", - "feedback_queue": "murfey_feedback", "rabbitmq_credentials": "/path/to/rabbitmq.yaml", - "murfey_db_credentials": "/path/to/murfey_db_credentials", - "crypto_key": "crypto_key", + "feedback_queue": "murfey_feedback", + "ispyb_credentials": str(mock_ispyb_credentials), } with open(config_file, "w") as f: json.dump(security_config, f) + return config_file From a78d2bbf5f2914d570ae1fb59db5406d3f789b01 Mon Sep 17 00:00:00 2001 From: Eu Pin Tien Date: Tue, 6 May 2025 15:03:23 +0100 Subject: [PATCH 06/47] Simplified use of security configuration fixture in 'test_run_repost_failed_calls()' --- tests/cli/test_repost_failed_calls.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/tests/cli/test_repost_failed_calls.py b/tests/cli/test_repost_failed_calls.py index 9a018fb4f..6ef2c7e81 100644 --- a/tests/cli/test_repost_failed_calls.py +++ b/tests/cli/test_repost_failed_calls.py @@ -6,7 +6,6 @@ from unittest import mock from murfey.cli import repost_failed_calls -from tests.conftest import mock_security_config_name @mock.patch("murfey.cli.repost_failed_calls.PikaTransport") @@ -167,12 +166,11 @@ def test_run_repost_failed_calls( mock_repost, mock_purge, mock_security_configuration, - tmp_path, ): mock_jwt.encode.return_value = "dummy_token" mock_purge.return_value = ["/path/to/msg1"] - config_file = tmp_path / mock_security_config_name + config_file = mock_security_configuration with open(config_file) as f: security_config = json.load(f) From 992f484e556ba31198bd3ea63d703c0451fdcc52 Mon Sep 17 00:00:00 2001 From: Eu Pin Tien Date: Tue, 6 May 2025 15:13:47 +0100 Subject: [PATCH 07/47] Renamed the Murfey engine imported for use in other tests --- .../spa/test_flush_spa_preprocess.py | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/tests/workflows/spa/test_flush_spa_preprocess.py b/tests/workflows/spa/test_flush_spa_preprocess.py index f55f9838c..408100d52 100644 --- a/tests/workflows/spa/test_flush_spa_preprocess.py +++ b/tests/workflows/spa/test_flush_spa_preprocess.py @@ -5,7 +5,7 @@ from murfey.util.db import DataCollectionGroup, GridSquare from murfey.util.models import GridSquareParameters from murfey.workflows.spa import flush_spa_preprocess -from tests import engine +from tests import murfey_db_engine @mock.patch("murfey.workflows.spa.flush_spa_preprocess._transport_object") @@ -18,7 +18,7 @@ def test_register_grid_square_update_add_locations(mock_transport, start_postgre session_id=2, tag="session_tag", ) - with Session(engine) as murfey_db: + with Session(murfey_db_engine) as murfey_db: murfey_db.add(grid_square) murfey_db.commit() @@ -32,14 +32,14 @@ def test_register_grid_square_update_add_locations(mock_transport, start_postgre ) # Run the registration - with Session(engine) as murfey_db: + with Session(murfey_db_engine) as murfey_db: flush_spa_preprocess.register_grid_square(2, 101, new_parameters, murfey_db) # Check this would have updated ispyb mock_transport.do_update_grid_square.assert_called_with(1, new_parameters) # Confirm the database was updated - with Session(engine) as murfey_db: + with Session(murfey_db_engine) as murfey_db: grid_square_final_parameters = murfey_db.exec(select(GridSquare)).one() assert grid_square_final_parameters.x_location == new_parameters.x_location assert grid_square_final_parameters.y_location == new_parameters.y_location @@ -65,7 +65,7 @@ def test_register_grid_square_update_add_nothing(mock_transport, start_postgres) x_stage_position=0.3, y_stage_position=0.4, ) - with Session(engine) as murfey_db: + with Session(murfey_db_engine) as murfey_db: murfey_db.add(grid_square) murfey_db.commit() @@ -73,14 +73,14 @@ def test_register_grid_square_update_add_nothing(mock_transport, start_postgres) new_parameters = GridSquareParameters(tag="session_tag") # Run the registration - with Session(engine) as murfey_db: + with Session(murfey_db_engine) as murfey_db: flush_spa_preprocess.register_grid_square(2, 101, new_parameters, murfey_db) # Check this would have updated ispyb mock_transport.do_update_grid_square.assert_called_with(1, new_parameters) # Confirm the database was not updated - with Session(engine) as murfey_db: + with Session(murfey_db_engine) as murfey_db: grid_square_final_parameters = murfey_db.exec(select(GridSquare)).one() assert grid_square_final_parameters.x_location == 0.1 assert grid_square_final_parameters.y_location == 0.2 @@ -99,7 +99,7 @@ def test_register_grid_square_insert_with_ispyb( tag="session_tag", atlas_id=90, ) - with Session(engine) as murfey_db: + with Session(murfey_db_engine) as murfey_db: murfey_db.add(grid_square) murfey_db.commit() @@ -126,14 +126,14 @@ def test_register_grid_square_insert_with_ispyb( ) # Run the registration - with Session(engine) as murfey_db: + with Session(murfey_db_engine) as murfey_db: flush_spa_preprocess.register_grid_square(2, 101, new_parameters, murfey_db) # Check this would have updated ispyb mock_transport.do_insert_grid_square.assert_called_with(90, 101, new_parameters) # Confirm the database entry was made - with Session(engine) as murfey_db: + with Session(murfey_db_engine) as murfey_db: grid_square_final_parameters = murfey_db.exec(select(GridSquare)).one() assert grid_square_final_parameters.id == 1 assert grid_square_final_parameters.name == 101 From 822050caf990007f3e6a12444d9524eb8d77b6c4 Mon Sep 17 00:00:00 2001 From: Eu Pin Tien Date: Tue, 6 May 2025 17:54:37 +0100 Subject: [PATCH 08/47] Added fixtures to create a mock ISPyB session --- tests/conftest.py | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/tests/conftest.py b/tests/conftest.py index 7413a9158..fcaa1c1db 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -2,7 +2,11 @@ from configparser import ConfigParser from pathlib import Path +import ispyb import pytest +from ispyb.sqlalchemy import url +from sqlalchemy import create_engine +from sqlalchemy.orm import scoped_session, sessionmaker from sqlmodel import Session from murfey.util.db import Session as MurfeySession @@ -70,3 +74,40 @@ def mock_security_configuration( with open(config_file, "w") as f: json.dump(security_config, f) return config_file + + +""" +======================================================================================= +Fixtures for setting up mock ISPyB database +======================================================================================= +These were adapted from the tests found at: +https://github.com/DiamondLightSource/ispyb-api/blob/main/tests/conftest.py +""" + + +@pytest.fixture +def ispyb_db(mock_ispyb_credentials): + with ispyb.open(mock_ispyb_credentials) as connection: + yield connection + + +@pytest.fixture(scope="session") +def ispyb_engine(mock_ispyb_credentials): + ispyb_engine = create_engine( + url=url(mock_ispyb_credentials), connect_args={"use_pure": True} + ) + yield ispyb_engine + ispyb_engine.dispose() + + +@pytest.fixture(scope="session") +def ispyb_session_factory(ispyb_engine): + return scoped_session(sessionmaker(bind=ispyb_engine)) + + +@pytest.fixture(scope="function") +def ispyb_session(ispyb_session_factory): + ispyb_session = ispyb_session_factory() + yield ispyb_session + ispyb_session.rollback() + ispyb_session.close() From b9513536c65872932bebc69c179b68a59791e39f Mon Sep 17 00:00:00 2001 From: Eu Pin Tien Date: Tue, 6 May 2025 17:56:33 +0100 Subject: [PATCH 09/47] Attempt at writing unit test for 'get_session_id()' function --- tests/server/test_ispyb.py | 42 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) create mode 100644 tests/server/test_ispyb.py diff --git a/tests/server/test_ispyb.py b/tests/server/test_ispyb.py new file mode 100644 index 000000000..da2ce4868 --- /dev/null +++ b/tests/server/test_ispyb.py @@ -0,0 +1,42 @@ +from ispyb.sqlalchemy import BLSession, Person, Proposal + +from murfey.server.ispyb import get_session_id + + +def test_get_session_id( + ispyb_session, +): + + # Create some values to put into BLSession and Proposal + # 'Person' is a required table + person_db_entry = Person( + login="murfey", + ) + ispyb_session.add(person_db_entry) + ispyb_session.commit() + + proposal_db_entry = Proposal( + personId=Person.personId, + proposalCode="cm", + proposalNumber="12345", + ) + ispyb_session.add(proposal_db_entry) + ispyb_session.commit() + + bl_session_db_entry = BLSession( + proposalId=proposal_db_entry.proposalId, + beamLineName="murfey", + visit_number=6, + ) + ispyb_session.add(bl_session_db_entry) + ispyb_session.commit() + + # Test function + result = get_session_id( + microscope="murfey", + proposal_code="cm", + proposal_number="12345", + visit_number="6", + db=ispyb_session, + ) + assert result == bl_session_db_entry.sessionId From 43b9bfdfe8497e799755fe75bd3b58bff99fc027 Mon Sep 17 00:00:00 2001 From: Eu Pin Tien Date: Tue, 6 May 2025 18:00:18 +0100 Subject: [PATCH 10/47] Adjusted scopes of common fixtures in 'conftest.py' --- tests/conftest.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index fcaa1c1db..f605069f1 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -25,7 +25,7 @@ def start_postgres(): murfey_db.commit() -@pytest.fixture() +@pytest.fixture(scope="session") def mock_client_configuration() -> ConfigParser: """ Returns the client-side configuration file as a pre-loaded ConfigParser object. @@ -39,7 +39,7 @@ def mock_client_configuration() -> ConfigParser: return config -@pytest.fixture() +@pytest.fixture(scope="session") def mock_ispyb_credentials(tmp_path: Path): creds_file = tmp_path / "ispyb_creds.cfg" ispyb_config = ConfigParser() @@ -56,7 +56,7 @@ def mock_ispyb_credentials(tmp_path: Path): return creds_file -@pytest.fixture() +@pytest.fixture(scope="session") def mock_security_configuration( tmp_path: Path, mock_ispyb_credentials: Path, @@ -85,7 +85,7 @@ def mock_security_configuration( """ -@pytest.fixture +@pytest.fixture(scope="session") def ispyb_db(mock_ispyb_credentials): with ispyb.open(mock_ispyb_credentials) as connection: yield connection @@ -105,7 +105,7 @@ def ispyb_session_factory(ispyb_engine): return scoped_session(sessionmaker(bind=ispyb_engine)) -@pytest.fixture(scope="function") +@pytest.fixture def ispyb_session(ispyb_session_factory): ispyb_session = ispyb_session_factory() yield ispyb_session From f0d7a398d75edbdfcd29e6bf2a15c93c8b4aab4b Mon Sep 17 00:00:00 2001 From: Eu Pin Tien Date: Tue, 6 May 2025 18:25:01 +0100 Subject: [PATCH 11/47] Added fixture to create a tmp_path Path that will persist for the test session --- tests/conftest.py | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index f605069f1..c2ddd6b48 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -14,6 +14,14 @@ from tests import murfey_db_engine, murfey_db_url +@pytest.fixture(scope="session") +def session_tmp_path(tmp_path_factory) -> Path: + """ + Creates a temporary path that persists for the entire test session + """ + return tmp_path_factory.mktemp("session_tmp") + + @pytest.fixture def start_postgres(): clear(murfey_db_url) @@ -40,8 +48,8 @@ def mock_client_configuration() -> ConfigParser: @pytest.fixture(scope="session") -def mock_ispyb_credentials(tmp_path: Path): - creds_file = tmp_path / "ispyb_creds.cfg" +def mock_ispyb_credentials(session_tmp_path: Path) -> Path: + creds_file = session_tmp_path / "ispyb_creds.cfg" ispyb_config = ConfigParser() # Use values from the GitHub workflow ISPyB config file ispyb_config["ispyb_sqlalchemy"] = { @@ -58,10 +66,10 @@ def mock_ispyb_credentials(tmp_path: Path): @pytest.fixture(scope="session") def mock_security_configuration( - tmp_path: Path, + session_tmp_path: Path, mock_ispyb_credentials: Path, -): - config_file = tmp_path / "security_config.yaml" +) -> Path: + config_file = session_tmp_path / "security_config.yaml" security_config = { "murfey_db_credentials": "/path/to/murfey_db_credentials", "crypto_key": "crypto_key", From 38eeee604e9e14b7cc5aab80ad51e52b9654270d Mon Sep 17 00:00:00 2001 From: Eu Pin Tien Date: Tue, 6 May 2025 18:34:23 +0100 Subject: [PATCH 12/47] Accidentally referred to the table class instead of the table instance with values inserted --- tests/server/test_ispyb.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/server/test_ispyb.py b/tests/server/test_ispyb.py index da2ce4868..fb317a8c1 100644 --- a/tests/server/test_ispyb.py +++ b/tests/server/test_ispyb.py @@ -16,7 +16,7 @@ def test_get_session_id( ispyb_session.commit() proposal_db_entry = Proposal( - personId=Person.personId, + personId=person_db_entry.personId, proposalCode="cm", proposalNumber="12345", ) From 6f80755dc7d31837fbb88a78ee3818b0a4ee7e2e Mon Sep 17 00:00:00 2001 From: Eu Pin Tien Date: Tue, 6 May 2025 18:40:26 +0100 Subject: [PATCH 13/47] Updated ISPyB database schema version --- .github/workflows/test.yml | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 1bbe2e327..a3a09032d 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -3,11 +3,12 @@ name: Build and test on: [push, pull_request] env: - DATABASE_SCHEMA: 4.2.1 # released 2024-08-19 + ISPYB_DATABASE_SCHEMA: 4.6.0 # Installs from GitHub # Versions: https://github.com/DiamondLightSource/ispyb-database/tags # Previous version(s): - # 4.1.0 + # 4.2.1 # released 2024-08-19 + # 4.1.0 # released 2024-03-26 permissions: contents: read @@ -53,10 +54,10 @@ jobs: runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 - - name: Download ISPyB DB schema v${{ env.DATABASE_SCHEMA }} for tests + - name: Download ISPyB DB schema v${{ env.ISPYB_DATABASE_SCHEMA }} for tests run: | mkdir database - wget -t 3 --waitretry=20 https://github.com/DiamondLightSource/ispyb-database/releases/download/v${{ env.DATABASE_SCHEMA }}/ispyb-database-${{ env.DATABASE_SCHEMA }}.tar.gz -O database/ispyb-database.tar.gz + wget -t 3 --waitretry=20 https://github.com/DiamondLightSource/ispyb-database/releases/download/v${{ env.ISPYB_DATABASE_SCHEMA }}/ispyb-database-${{ env.ISPYB_DATABASE_SCHEMA }}.tar.gz -O database/ispyb-database.tar.gz - name: Store database artifact uses: actions/upload-artifact@v4 with: From d0e1631d43b06dbaec36d1057771ecfa49003dce Mon Sep 17 00:00:00 2001 From: Eu Pin Tien Date: Tue, 6 May 2025 18:52:00 +0100 Subject: [PATCH 14/47] Trying mariadb v10.6, which matches the version used by dependencies --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4bb24e03a..bf3986384 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -17,7 +17,7 @@ jobs: python-version: ["3.9", "3.10", "3.11"] services: mariadb: - image: mariadb:11.7.2 # released 2024-05-06 + image: mariadb:10.6 # released 2024-05-06 # Pulls image from DockerHub # Docker images: https://hub.docker.com/_/mariadb # Previous version(s): From 30e0e9f4ca183ea44920f5d00c8c4ff0c116a9a7 Mon Sep 17 00:00:00 2001 From: Eu Pin Tien Date: Tue, 6 May 2025 19:03:23 +0100 Subject: [PATCH 15/47] Dynamically remove '\' characters from comments in SQL files to allow MariaDB to read them correctly --- .github/workflows/ci.yml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index bf3986384..6c6972eab 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -101,6 +101,10 @@ jobs: schemas/ispyb/routines.sql \ grants/ispyb_processing.sql \ grants/ispyb_import.sql; do + + echo "Patching ${f} to fix CLI escape issues..." + sed -i 's/\\-/-/g' "$f" + echo Importing ${f}... mariadb --defaults-file=.my.cnf < $f done From 84fb9bd6d59b3e89ef7352aa1e5f36f23f4e9fd7 Mon Sep 17 00:00:00 2001 From: Eu Pin Tien Date: Wed, 7 May 2025 09:17:27 +0100 Subject: [PATCH 16/47] Updated names of CI workflow steps; removed ISPYB_CREDENTIALS variable from CI workflow --- .github/workflows/ci.yml | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6c6972eab..cf7369fe1 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -41,6 +41,7 @@ jobs: steps: - uses: actions/checkout@v4 + - name: Use Python ${{ matrix.python-version }} uses: actions/setup-python@v5 with: @@ -67,13 +68,13 @@ jobs: docker run --detach --name rabbitmq -p 127.0.0.1:5672:5672 -p 127.0.0.1:15672:15672 test-rabbitmq docker container list -a - - name: Get ispyb database + - name: Get ISPyB database uses: actions/download-artifact@v4 with: name: database path: database/ - - name: Install package + - name: Install Murfey run: | set -eux pip install --disable-pip-version-check -e "."[cicd,client,server,developer] @@ -84,7 +85,7 @@ jobs: mysql-version: "11.3" auto-start: false - - name: Set up test ipsyb database + - name: Set up test ISPyB database run: | set -eu cp ".github/workflows/config/my.cnf" .my.cnf @@ -102,12 +103,13 @@ jobs: grants/ispyb_processing.sql \ grants/ispyb_import.sql; do - echo "Patching ${f} to fix CLI escape issues..." + echo "Patching ${f} in SQL files to fix CLI escape issues..." sed -i 's/\\-/-/g' "$f" - echo Importing ${f}... + echo "Importing ${f}..." mariadb --defaults-file=.my.cnf < $f done + mariadb --defaults-file=.my.cnf -e "CREATE USER ispyb_api@'%' IDENTIFIED BY 'password_1234'; GRANT ispyb_processing to ispyb_api@'%'; GRANT ispyb_import to ispyb_api@'%'; SET DEFAULT ROLE ispyb_processing FOR ispyb_api@'%';" mariadb --defaults-file=.my.cnf -e "CREATE USER ispyb_api_future@'%' IDENTIFIED BY 'password_4321'; GRANT SELECT ON ispybtest.* to ispyb_api_future@'%';" mariadb --defaults-file=.my.cnf -e "CREATE USER ispyb_api_sqlalchemy@'%' IDENTIFIED BY 'password_5678'; GRANT SELECT ON ispybtest.* to ispyb_api_sqlalchemy@'%'; GRANT INSERT ON ispybtest.* to ispyb_api_sqlalchemy@'%'; GRANT UPDATE ON ispybtest.* to ispyb_api_sqlalchemy@'%';" @@ -116,7 +118,7 @@ jobs: - name: Check RabbitMQ is alive run: wget -t 10 -w 1 http://127.0.0.1:15672 -O - - - name: Run tests + - name: Run Murfey tests env: POSTGRES_HOST: localhost POSTGRES_PORT: 5432 @@ -124,10 +126,9 @@ jobs: POSTGRES_PASSWORD: psql_pwd POSTGRES_USER: psql_user run: | - export ISPYB_CREDENTIALS=".github/workflows/config/ispyb.cfg" PYTHONDEVMODE=1 pytest -v -ra --cov=murfey --cov-report=xml --cov-branch - - name: Upload to Codecov + - name: Upload test results to Codecov uses: codecov/codecov-action@v5 with: name: ${{ matrix.python-version }} From 7282ebf966fe0a7a6cd78fe73d60d56f5c998e75 Mon Sep 17 00:00:00 2001 From: Eu Pin Tien Date: Wed, 7 May 2025 09:28:25 +0100 Subject: [PATCH 17/47] Switched back to using latest stable version of MariaDB for CI workflow --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index cf7369fe1..a2a490928 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -17,7 +17,7 @@ jobs: python-version: ["3.9", "3.10", "3.11"] services: mariadb: - image: mariadb:10.6 # released 2024-05-06 + image: mariadb:11.7.2 # released 2024-05-06 # Pulls image from DockerHub # Docker images: https://hub.docker.com/_/mariadb # Previous version(s): From 58254575056bb00ec62e222f73530c8db467b423 Mon Sep 17 00:00:00 2001 From: Eu Pin Tien Date: Wed, 7 May 2025 11:36:31 +0100 Subject: [PATCH 18/47] Added a class to 'conftest.py' storing the database parameters needed for an example visit; populated the ISPyB database in the fixture instead of in the test --- tests/conftest.py | 91 +++++++++++++++++++++++++++++++++++++---------- 1 file changed, 73 insertions(+), 18 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index c2ddd6b48..3f0204190 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,10 +1,11 @@ import json from configparser import ConfigParser from pathlib import Path +from typing import Generator import ispyb import pytest -from ispyb.sqlalchemy import url +from ispyb.sqlalchemy import BLSession, Person, Proposal, url from sqlalchemy import create_engine from sqlalchemy.orm import scoped_session, sessionmaker from sqlmodel import Session @@ -22,17 +23,6 @@ def session_tmp_path(tmp_path_factory) -> Path: return tmp_path_factory.mktemp("session_tmp") -@pytest.fixture -def start_postgres(): - clear(murfey_db_url) - setup(murfey_db_url) - - murfey_session = MurfeySession(id=2, name="cm12345-6") - with Session(murfey_db_engine) as murfey_db: - murfey_db.add(murfey_session) - murfey_db.commit() - - @pytest.fixture(scope="session") def mock_client_configuration() -> ConfigParser: """ @@ -93,8 +83,27 @@ def mock_security_configuration( """ +class ExampleVisit: + """ + This is a class to store information that will common to all database entries for + a particular Murfey session, to enable ease of replication when creating database + fixtures. + """ + + # Visit-related (ISPyB & Murfey) + instrument_name = "i01" + proposal_code = "cm" + proposal_number = 12345 + visit_number = 6 + + # Person (ISPyB) + given_name = "Eliza" + family_name = "Murfey" + login = "murfey123" + + @pytest.fixture(scope="session") -def ispyb_db(mock_ispyb_credentials): +def ispyb_db_connection(mock_ispyb_credentials): with ispyb.open(mock_ispyb_credentials) as connection: yield connection @@ -114,8 +123,54 @@ def ispyb_session_factory(ispyb_engine): @pytest.fixture -def ispyb_session(ispyb_session_factory): - ispyb_session = ispyb_session_factory() - yield ispyb_session - ispyb_session.rollback() - ispyb_session.close() +def ispyb_db(ispyb_session_factory) -> Generator[Session, None, None]: + # Get a new session from the session factory + ispyb_db: Session = ispyb_session_factory() + + # Populate the ISPyB table with some default values + person_db_entry = Person( + givenName=ExampleVisit.given_name, + familyName=ExampleVisit.family_name, + login=ExampleVisit.login, + ) + proposal_db_entry = Proposal( + personId=person_db_entry.personId, + proposalCode=ExampleVisit.proposal_code, + proposalNumber=str(ExampleVisit.proposal_number), + ) + bl_session_db_entry = BLSession( + proposalId=proposal_db_entry.proposalId, + beamLineName=ExampleVisit.instrument_name, + visit_number=ExampleVisit.visit_number, + ) + ispyb_db.add_all( + [ + person_db_entry, + proposal_db_entry, + bl_session_db_entry, + ] + ) + ispyb_db.commit() + yield ispyb_db # Yield the Session and pass processing over to other function + + # Tidying up + ispyb_db.rollback() + ispyb_db.close() + + +""" +======================================================================================= +Fixtures for setting up mock Murfey database +======================================================================================= +""" + + +@pytest.fixture +def start_postgres(): + clear(murfey_db_url) + setup(murfey_db_url) + + murfey_session = MurfeySession(id=2, name="cm12345-6") + with Session(murfey_db_engine) as murfey_db: + murfey_db.add(murfey_session) + murfey_db.commit() From e82bd67aefcade03cb01c15fd085cd7d79ca958a Mon Sep 17 00:00:00 2001 From: Eu Pin Tien Date: Wed, 7 May 2025 11:37:23 +0100 Subject: [PATCH 19/47] Simplified the test for the 'get_session_id()' function, now that the database is pre-populated --- tests/server/test_ispyb.py | 48 ++++++++++++++++---------------------- 1 file changed, 20 insertions(+), 28 deletions(-) diff --git a/tests/server/test_ispyb.py b/tests/server/test_ispyb.py index fb317a8c1..801510020 100644 --- a/tests/server/test_ispyb.py +++ b/tests/server/test_ispyb.py @@ -1,42 +1,34 @@ -from ispyb.sqlalchemy import BLSession, Person, Proposal +from ispyb.sqlalchemy import BLSession, Proposal +from sqlmodel import Session, select from murfey.server.ispyb import get_session_id +from tests.conftest import ExampleVisit def test_get_session_id( - ispyb_session, + ispyb_db: Session, ): - - # Create some values to put into BLSession and Proposal - # 'Person' is a required table - person_db_entry = Person( - login="murfey", - ) - ispyb_session.add(person_db_entry) - ispyb_session.commit() - - proposal_db_entry = Proposal( - personId=person_db_entry.personId, - proposalCode="cm", - proposalNumber="12345", - ) - ispyb_session.add(proposal_db_entry) - ispyb_session.commit() - - bl_session_db_entry = BLSession( - proposalId=proposal_db_entry.proposalId, - beamLineName="murfey", - visit_number=6, + # Manually get the BLSession ID for comparison + result = ( + ispyb_db.exec( + select(BLSession) + .join(Proposal) + .where(BLSession.proposalId == Proposal.proposalId) + .where(BLSession.beamLineName == ExampleVisit.instrument_name) + .where(Proposal.proposalCode == ExampleVisit.proposal_code) + .where(Proposal.proposalNumber == str(ExampleVisit.proposal_number)) + .where(BLSession.visit_number == ExampleVisit.visit_number) + ) + .one() + .sessionId ) - ispyb_session.add(bl_session_db_entry) - ispyb_session.commit() # Test function - result = get_session_id( + session_id = get_session_id( microscope="murfey", proposal_code="cm", proposal_number="12345", visit_number="6", - db=ispyb_session, + db=ispyb_db, ) - assert result == bl_session_db_entry.sessionId + assert result == session_id From 46c47a1b5072fc784935f0504f7bdba158efa969 Mon Sep 17 00:00:00 2001 From: Eu Pin Tien Date: Wed, 7 May 2025 11:49:35 +0100 Subject: [PATCH 20/47] Changed variable name --- tests/server/test_ispyb.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/server/test_ispyb.py b/tests/server/test_ispyb.py index 801510020..b50c5c69f 100644 --- a/tests/server/test_ispyb.py +++ b/tests/server/test_ispyb.py @@ -9,7 +9,7 @@ def test_get_session_id( ispyb_db: Session, ): # Manually get the BLSession ID for comparison - result = ( + query = ( ispyb_db.exec( select(BLSession) .join(Proposal) @@ -24,11 +24,11 @@ def test_get_session_id( ) # Test function - session_id = get_session_id( + result = get_session_id( microscope="murfey", proposal_code="cm", proposal_number="12345", visit_number="6", db=ispyb_db, ) - assert result == session_id + assert query == result From 464b32ef4a30371b33cb5d705818fb6d2def1b52 Mon Sep 17 00:00:00 2001 From: Eu Pin Tien Date: Wed, 7 May 2025 11:49:48 +0100 Subject: [PATCH 21/47] Add and commit tables one-by-one --- tests/conftest.py | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index 3f0204190..cfce90751 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -133,25 +133,27 @@ def ispyb_db(ispyb_session_factory) -> Generator[Session, None, None]: familyName=ExampleVisit.family_name, login=ExampleVisit.login, ) + ispyb_db.add(person_db_entry) + ispyb_db.commit() + proposal_db_entry = Proposal( personId=person_db_entry.personId, proposalCode=ExampleVisit.proposal_code, proposalNumber=str(ExampleVisit.proposal_number), ) + ispyb_db.add(proposal_db_entry) + ispyb_db.commit() + bl_session_db_entry = BLSession( proposalId=proposal_db_entry.proposalId, beamLineName=ExampleVisit.instrument_name, visit_number=ExampleVisit.visit_number, ) - ispyb_db.add_all( - [ - person_db_entry, - proposal_db_entry, - bl_session_db_entry, - ] - ) + ispyb_db.add(bl_session_db_entry) ispyb_db.commit() - yield ispyb_db # Yield the Session and pass processing over to other function + + # Yield the Session and pass processing over to other function + yield ispyb_db # Tidying up ispyb_db.rollback() From 20ccaaa30f2d4b36ebbe5de30e81956b2e5d98c0 Mon Sep 17 00:00:00 2001 From: Eu Pin Tien Date: Wed, 7 May 2025 12:02:54 +0100 Subject: [PATCH 22/47] ISPyB tables are specifically 'sqlalchemy.orm.Session' objects; indicate so --- tests/conftest.py | 8 ++++---- tests/server/test_ispyb.py | 5 +++-- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index cfce90751..11977374f 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -7,8 +7,8 @@ import pytest from ispyb.sqlalchemy import BLSession, Person, Proposal, url from sqlalchemy import create_engine +from sqlalchemy.orm import Session as SQLAlchemySession from sqlalchemy.orm import scoped_session, sessionmaker -from sqlmodel import Session from murfey.util.db import Session as MurfeySession from murfey.util.db import clear, setup @@ -123,9 +123,9 @@ def ispyb_session_factory(ispyb_engine): @pytest.fixture -def ispyb_db(ispyb_session_factory) -> Generator[Session, None, None]: +def ispyb_db(ispyb_session_factory) -> Generator[SQLAlchemySession, None, None]: # Get a new session from the session factory - ispyb_db: Session = ispyb_session_factory() + ispyb_db: SQLAlchemySession = ispyb_session_factory() # Populate the ISPyB table with some default values person_db_entry = Person( @@ -173,6 +173,6 @@ def start_postgres(): setup(murfey_db_url) murfey_session = MurfeySession(id=2, name="cm12345-6") - with Session(murfey_db_engine) as murfey_db: + with SQLAlchemySession(murfey_db_engine) as murfey_db: murfey_db.add(murfey_session) murfey_db.commit() diff --git a/tests/server/test_ispyb.py b/tests/server/test_ispyb.py index b50c5c69f..15d952ce5 100644 --- a/tests/server/test_ispyb.py +++ b/tests/server/test_ispyb.py @@ -1,5 +1,6 @@ from ispyb.sqlalchemy import BLSession, Proposal -from sqlmodel import Session, select +from sqlalchemy import select +from sqlalchemy.orm import Session from murfey.server.ispyb import get_session_id from tests.conftest import ExampleVisit @@ -10,7 +11,7 @@ def test_get_session_id( ): # Manually get the BLSession ID for comparison query = ( - ispyb_db.exec( + ispyb_db.execute( select(BLSession) .join(Proposal) .where(BLSession.proposalId == Proposal.proposalId) From 32fe03ac3c956421f314aaa41ed6a1460270e86d Mon Sep 17 00:00:00 2001 From: Eu Pin Tien Date: Wed, 7 May 2025 13:09:59 +0100 Subject: [PATCH 23/47] Table selection in 'sqlalchemy' is done using 'scalar_one()', not 'one()' --- tests/server/test_ispyb.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/server/test_ispyb.py b/tests/server/test_ispyb.py index 15d952ce5..78dd46463 100644 --- a/tests/server/test_ispyb.py +++ b/tests/server/test_ispyb.py @@ -20,7 +20,7 @@ def test_get_session_id( .where(Proposal.proposalNumber == str(ExampleVisit.proposal_number)) .where(BLSession.visit_number == ExampleVisit.visit_number) ) - .one() + .scalar_one() .sessionId ) From 8336fbd16622baa17de6737211c41b75a2d7f610 Mon Sep 17 00:00:00 2001 From: Eu Pin Tien Date: Wed, 7 May 2025 13:16:08 +0100 Subject: [PATCH 24/47] Forgot to update parameters passed to 'get_session_id()' in test --- tests/server/test_ispyb.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/server/test_ispyb.py b/tests/server/test_ispyb.py index 78dd46463..16f8a76e9 100644 --- a/tests/server/test_ispyb.py +++ b/tests/server/test_ispyb.py @@ -26,10 +26,10 @@ def test_get_session_id( # Test function result = get_session_id( - microscope="murfey", - proposal_code="cm", - proposal_number="12345", - visit_number="6", + microscope=ExampleVisit.instrument_name, + proposal_code=ExampleVisit.proposal_code, + proposal_number=str(ExampleVisit.proposal_number), + visit_number=str(ExampleVisit.visit_number), db=ispyb_db, ) assert query == result From cabec6f63f3f6ff4456fabc98426c0f1ed68bd6a Mon Sep 17 00:00:00 2001 From: Eu Pin Tien Date: Wed, 7 May 2025 13:35:41 +0100 Subject: [PATCH 25/47] Added unit test for 'get_proposal_id()' function --- tests/server/test_ispyb.py | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/tests/server/test_ispyb.py b/tests/server/test_ispyb.py index 16f8a76e9..fd289ef35 100644 --- a/tests/server/test_ispyb.py +++ b/tests/server/test_ispyb.py @@ -2,7 +2,7 @@ from sqlalchemy import select from sqlalchemy.orm import Session -from murfey.server.ispyb import get_session_id +from murfey.server.ispyb import get_proposal_id, get_session_id from tests.conftest import ExampleVisit @@ -33,3 +33,25 @@ def test_get_session_id( db=ispyb_db, ) assert query == result + + +def test_get_proposal_id( + ispyb_db: Session, +): + # Manually query the Proposal ID + query = ( + ispyb_db.execute( + select(Proposal) + .where(Proposal.proposalCode == ExampleVisit.proposal_code) + .where(Proposal.proposalNumber == ExampleVisit.proposal_number) + ) + .scalar_one() + .proposalId + ) + + # Test function + result = get_proposal_id( + proposal_code=ExampleVisit.proposal_code, + proposal_number=ExampleVisit.proposal_number, + ) + assert query == result From 86e8a07d2dfd94fa85ea838aaec0cfe41f34ae3a Mon Sep 17 00:00:00 2001 From: Eu Pin Tien Date: Wed, 7 May 2025 13:37:54 +0100 Subject: [PATCH 26/47] Forgot to add ISPyB database a a parameter --- tests/server/test_ispyb.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/server/test_ispyb.py b/tests/server/test_ispyb.py index fd289ef35..de3a25b44 100644 --- a/tests/server/test_ispyb.py +++ b/tests/server/test_ispyb.py @@ -53,5 +53,6 @@ def test_get_proposal_id( result = get_proposal_id( proposal_code=ExampleVisit.proposal_code, proposal_number=ExampleVisit.proposal_number, + db=ispyb_db, ) assert query == result From 3c656de26eb8ee5f56d84c59a9fb8e38ed865a0e Mon Sep 17 00:00:00 2001 From: Eu Pin Tien Date: Wed, 7 May 2025 13:54:31 +0100 Subject: [PATCH 27/47] Try using 'begin_nested()' to roll back ISPyB database after tests --- tests/conftest.py | 63 +++++++++++++++++++++++++---------------------- 1 file changed, 33 insertions(+), 30 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index 11977374f..1a5bb5cae 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -126,38 +126,41 @@ def ispyb_session_factory(ispyb_engine): def ispyb_db(ispyb_session_factory) -> Generator[SQLAlchemySession, None, None]: # Get a new session from the session factory ispyb_db: SQLAlchemySession = ispyb_session_factory() - - # Populate the ISPyB table with some default values - person_db_entry = Person( - givenName=ExampleVisit.given_name, - familyName=ExampleVisit.family_name, - login=ExampleVisit.login, - ) - ispyb_db.add(person_db_entry) - ispyb_db.commit() - - proposal_db_entry = Proposal( - personId=person_db_entry.personId, - proposalCode=ExampleVisit.proposal_code, - proposalNumber=str(ExampleVisit.proposal_number), - ) - ispyb_db.add(proposal_db_entry) - ispyb_db.commit() - - bl_session_db_entry = BLSession( - proposalId=proposal_db_entry.proposalId, - beamLineName=ExampleVisit.instrument_name, - visit_number=ExampleVisit.visit_number, - ) - ispyb_db.add(bl_session_db_entry) - ispyb_db.commit() - - # Yield the Session and pass processing over to other function - yield ispyb_db + save_point = ispyb_db.begin_nested() # Checkpoint to roll back database to + + try: + # Populate the ISPyB table with some default values + person_db_entry = Person( + givenName=ExampleVisit.given_name, + familyName=ExampleVisit.family_name, + login=ExampleVisit.login, + ) + ispyb_db.add(person_db_entry) + ispyb_db.commit() + + proposal_db_entry = Proposal( + personId=person_db_entry.personId, + proposalCode=ExampleVisit.proposal_code, + proposalNumber=str(ExampleVisit.proposal_number), + ) + ispyb_db.add(proposal_db_entry) + ispyb_db.commit() + + bl_session_db_entry = BLSession( + proposalId=proposal_db_entry.proposalId, + beamLineName=ExampleVisit.instrument_name, + visit_number=ExampleVisit.visit_number, + ) + ispyb_db.add(bl_session_db_entry) + ispyb_db.commit() + + # Yield the Session and pass processing over to other function + yield ispyb_db # Tidying up - ispyb_db.rollback() - ispyb_db.close() + finally: + save_point.rollback() + ispyb_db.close() """ From 06db6fa02f99eed49ca1090b945840589b448098 Mon Sep 17 00:00:00 2001 From: Eu Pin Tien Date: Wed, 7 May 2025 14:52:00 +0100 Subject: [PATCH 28/47] Moved the insertion of starting ISPyB values to the session factory fixture, and made the insertions idempotent --- tests/conftest.py | 113 ++++++++++++++++++++++++++++++---------------- 1 file changed, 75 insertions(+), 38 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index 1a5bb5cae..cbf8abbb4 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,12 +1,13 @@ import json from configparser import ConfigParser from pathlib import Path -from typing import Generator +from typing import Any, Generator, Type, TypeVar import ispyb import pytest from ispyb.sqlalchemy import BLSession, Person, Proposal, url -from sqlalchemy import create_engine +from sqlalchemy import and_, create_engine, select +from sqlalchemy.ext.declarative import DeclarativeMeta from sqlalchemy.orm import Session as SQLAlchemySession from sqlalchemy.orm import scoped_session, sessionmaker @@ -117,50 +118,86 @@ def ispyb_engine(mock_ispyb_credentials): ispyb_engine.dispose() +SQLAlchemyTable = TypeVar("SQLAlchemyTable", bound=DeclarativeMeta) + + +def get_or_create_db_entry( + session: SQLAlchemySession, + table: Type[SQLAlchemyTable], + lookup_kwargs: dict[str, Any] = {}, + insert_kwargs: dict[str, Any] = {}, +) -> SQLAlchemyTable: + """ + Helper function to facilitate looking up SQLAlchemy tables for + matching entries. Returns the entry if it exists and creates it + if it doesn't. + """ + + # if lookup kwargs are provided, check if entry exists + if lookup_kwargs: + conditions = [ + getattr(table, key) == value for key, value in lookup_kwargs.items() + ] + entry = session.execute(select(table).where(and_(*conditions))) + if entry: + return entry + # If not present, create and return new entry + # Use new kwargs if provided; otherwise, use lookup kwargs + insert_kwargs = insert_kwargs or lookup_kwargs + entry = table(**insert_kwargs) + session.add(entry) + session.commit() + return entry + + @pytest.fixture(scope="session") def ispyb_session_factory(ispyb_engine): - return scoped_session(sessionmaker(bind=ispyb_engine)) + factory = scoped_session(sessionmaker(bind=ispyb_engine)) + ispyb_db = factory() + + # Populate the ISPyB table with some initial values + # Return existing table entry if already present + person_db_entry = get_or_create_db_entry( + session=ispyb_db, + table=Person, + lookup_kwargs={ + "givenName": ExampleVisit.given_name, + "familyName": ExampleVisit.family_name, + "login": ExampleVisit.login, + }, + ) + proposal_db_entry = get_or_create_db_entry( + session=ispyb_db, + table=Proposal, + lookup_kwargs={ + "personId": person_db_entry.personId, + "proposalCode": ExampleVisit.proposal_code, + "proposalNumber": str(ExampleVisit.proposal_number), + }, + ) + bl_session_db_entry = get_or_create_db_entry( + session=ispyb_db, + table=BLSession, + lookup_kwargs={ + "proposalId": proposal_db_entry.proposalId, + "beamLineName": ExampleVisit.instrument_name, + "visit_number": ExampleVisit.visit_number, + }, + ) + ispyb_db.add(bl_session_db_entry) + ispyb_db.commit() + + ispyb_db.close() + return factory # Return its current state @pytest.fixture def ispyb_db(ispyb_session_factory) -> Generator[SQLAlchemySession, None, None]: # Get a new session from the session factory ispyb_db: SQLAlchemySession = ispyb_session_factory() - save_point = ispyb_db.begin_nested() # Checkpoint to roll back database to - - try: - # Populate the ISPyB table with some default values - person_db_entry = Person( - givenName=ExampleVisit.given_name, - familyName=ExampleVisit.family_name, - login=ExampleVisit.login, - ) - ispyb_db.add(person_db_entry) - ispyb_db.commit() - - proposal_db_entry = Proposal( - personId=person_db_entry.personId, - proposalCode=ExampleVisit.proposal_code, - proposalNumber=str(ExampleVisit.proposal_number), - ) - ispyb_db.add(proposal_db_entry) - ispyb_db.commit() - - bl_session_db_entry = BLSession( - proposalId=proposal_db_entry.proposalId, - beamLineName=ExampleVisit.instrument_name, - visit_number=ExampleVisit.visit_number, - ) - ispyb_db.add(bl_session_db_entry) - ispyb_db.commit() - - # Yield the Session and pass processing over to other function - yield ispyb_db - - # Tidying up - finally: - save_point.rollback() - ispyb_db.close() + yield ispyb_db + ispyb_db.rollback() + ispyb_db.close() """ From af829e7de904de5f972f0d4c1e22722bd5926a71 Mon Sep 17 00:00:00 2001 From: Eu Pin Tien Date: Wed, 7 May 2025 14:54:31 +0100 Subject: [PATCH 29/47] Forgot to extract results of the database search --- tests/conftest.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/conftest.py b/tests/conftest.py index cbf8abbb4..c1e8b3552 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -138,7 +138,9 @@ def get_or_create_db_entry( conditions = [ getattr(table, key) == value for key, value in lookup_kwargs.items() ] - entry = session.execute(select(table).where(and_(*conditions))) + entry = ( + session.execute(select(table).where(and_(*conditions))).scalars().first() + ) if entry: return entry # If not present, create and return new entry From a47549077ba0f3122be90662fdadc3d2b5a052e9 Mon Sep 17 00:00:00 2001 From: Eu Pin Tien Date: Wed, 7 May 2025 15:23:21 +0100 Subject: [PATCH 30/47] Updated fixture name from 'ispyb_db' to 'ispyb_db_session' --- tests/conftest.py | 41 +++++++++++++++++++++----------------- tests/server/test_ispyb.py | 12 +++++------ 2 files changed, 29 insertions(+), 24 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index c1e8b3552..9526ec01b 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -128,9 +128,9 @@ def get_or_create_db_entry( insert_kwargs: dict[str, Any] = {}, ) -> SQLAlchemyTable: """ - Helper function to facilitate looking up SQLAlchemy tables for - matching entries. Returns the entry if it exists and creates it - if it doesn't. + Helper function to facilitate looking up or creating SQLAlchemy table entries. + Returns the entry if a match based on the lookup criteria is found, otherwise + creates and returns a new entry. """ # if lookup kwargs are provided, check if entry exists @@ -143,6 +143,7 @@ def get_or_create_db_entry( ) if entry: return entry + # If not present, create and return new entry # Use new kwargs if provided; otherwise, use lookup kwargs insert_kwargs = insert_kwargs or lookup_kwargs @@ -153,14 +154,14 @@ def get_or_create_db_entry( @pytest.fixture(scope="session") -def ispyb_session_factory(ispyb_engine): +def ispyb_db_session_factory(ispyb_engine): factory = scoped_session(sessionmaker(bind=ispyb_engine)) - ispyb_db = factory() # Populate the ISPyB table with some initial values # Return existing table entry if already present + ispyb_db_session = factory() person_db_entry = get_or_create_db_entry( - session=ispyb_db, + session=ispyb_db_session, table=Person, lookup_kwargs={ "givenName": ExampleVisit.given_name, @@ -169,7 +170,7 @@ def ispyb_session_factory(ispyb_engine): }, ) proposal_db_entry = get_or_create_db_entry( - session=ispyb_db, + session=ispyb_db_session, table=Proposal, lookup_kwargs={ "personId": person_db_entry.personId, @@ -177,8 +178,8 @@ def ispyb_session_factory(ispyb_engine): "proposalNumber": str(ExampleVisit.proposal_number), }, ) - bl_session_db_entry = get_or_create_db_entry( - session=ispyb_db, + _ = get_or_create_db_entry( + session=ispyb_db_session, table=BLSession, lookup_kwargs={ "proposalId": proposal_db_entry.proposalId, @@ -186,20 +187,24 @@ def ispyb_session_factory(ispyb_engine): "visit_number": ExampleVisit.visit_number, }, ) - ispyb_db.add(bl_session_db_entry) - ispyb_db.commit() - - ispyb_db.close() + ispyb_db_session.close() return factory # Return its current state @pytest.fixture -def ispyb_db(ispyb_session_factory) -> Generator[SQLAlchemySession, None, None]: +def ispyb_db_session( + ispyb_db_session_factory, +) -> Generator[SQLAlchemySession, None, None]: + # Get a new session from the session factory - ispyb_db: SQLAlchemySession = ispyb_session_factory() - yield ispyb_db - ispyb_db.rollback() - ispyb_db.close() + ispyb_db_session: SQLAlchemySession = ispyb_db_session_factory() + + # Let other function run + yield ispyb_db_session + + # Tidy up after function is complete + ispyb_db_session.rollback() + ispyb_db_session.close() """ diff --git a/tests/server/test_ispyb.py b/tests/server/test_ispyb.py index de3a25b44..29cb8f3c2 100644 --- a/tests/server/test_ispyb.py +++ b/tests/server/test_ispyb.py @@ -7,11 +7,11 @@ def test_get_session_id( - ispyb_db: Session, + ispyb_db_session: Session, ): # Manually get the BLSession ID for comparison query = ( - ispyb_db.execute( + ispyb_db_session.execute( select(BLSession) .join(Proposal) .where(BLSession.proposalId == Proposal.proposalId) @@ -30,17 +30,17 @@ def test_get_session_id( proposal_code=ExampleVisit.proposal_code, proposal_number=str(ExampleVisit.proposal_number), visit_number=str(ExampleVisit.visit_number), - db=ispyb_db, + db=ispyb_db_session, ) assert query == result def test_get_proposal_id( - ispyb_db: Session, + ispyb_db_session: Session, ): # Manually query the Proposal ID query = ( - ispyb_db.execute( + ispyb_db_session.execute( select(Proposal) .where(Proposal.proposalCode == ExampleVisit.proposal_code) .where(Proposal.proposalNumber == ExampleVisit.proposal_number) @@ -53,6 +53,6 @@ def test_get_proposal_id( result = get_proposal_id( proposal_code=ExampleVisit.proposal_code, proposal_number=ExampleVisit.proposal_number, - db=ispyb_db, + db=ispyb_db_session, ) assert query == result From 6b6575cd727c457df1c8bd326b7e9ea65a76f595 Mon Sep 17 00:00:00 2001 From: Eu Pin Tien Date: Wed, 7 May 2025 16:27:56 +0100 Subject: [PATCH 31/47] Tried alternate way of setting up test ISPyB database --- tests/conftest.py | 55 ++++++++++++++++++++++++++++++++++++----------- 1 file changed, 42 insertions(+), 13 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index 9526ec01b..358802022 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -6,10 +6,10 @@ import ispyb import pytest from ispyb.sqlalchemy import BLSession, Person, Proposal, url -from sqlalchemy import and_, create_engine, select +from sqlalchemy import Engine, RootTransaction, and_, create_engine, event, select from sqlalchemy.ext.declarative import DeclarativeMeta from sqlalchemy.orm import Session as SQLAlchemySession -from sqlalchemy.orm import scoped_session, sessionmaker +from sqlalchemy.orm import sessionmaker from murfey.util.db import Session as MurfeySession from murfey.util.db import clear, setup @@ -118,6 +118,11 @@ def ispyb_engine(mock_ispyb_credentials): ispyb_engine.dispose() +@pytest.fixture(scope="session") +def ispyb_db_session_factory(ispyb_engine): + return sessionmaker(bind=ispyb_engine, expire_on_commit=False) + + SQLAlchemyTable = TypeVar("SQLAlchemyTable", bound=DeclarativeMeta) @@ -154,12 +159,11 @@ def get_or_create_db_entry( @pytest.fixture(scope="session") -def ispyb_db_session_factory(ispyb_engine): - factory = scoped_session(sessionmaker(bind=ispyb_engine)) +def seed_ispyb_db(ispyb_db_session_factory): # Populate the ISPyB table with some initial values # Return existing table entry if already present - ispyb_db_session = factory() + ispyb_db_session: SQLAlchemySession = ispyb_db_session_factory() person_db_entry = get_or_create_db_entry( session=ispyb_db_session, table=Person, @@ -188,23 +192,48 @@ def ispyb_db_session_factory(ispyb_engine): }, ) ispyb_db_session.close() - return factory # Return its current state + + +def restart_savepoint(session: SQLAlchemySession, transaction: RootTransaction): + """ + Re-establish a SAVEPOINT after a nested transaction is committed or rolled back. + This helps to maintain isolation across different test cases. + """ + if transaction.nested and not transaction._parent.nested: + session.begin_nested() + + +def attach_event_listener(session: SQLAlchemySession): + """ + Attach the restart_savepoint function as an event listener for after_transaction_end + """ + event.listen(session, "after_transaction_end", restart_savepoint) @pytest.fixture def ispyb_db_session( ispyb_db_session_factory, + ispyb_engine: Engine, + seed_ispyb_db, ) -> Generator[SQLAlchemySession, None, None]: + """ + Returns a test-safe session that wraps each test in a rollback-safe SAVEPOINT. + """ + connection = ispyb_engine.connect() + transaction = connection.begin() # Outer transaction - # Get a new session from the session factory - ispyb_db_session: SQLAlchemySession = ispyb_db_session_factory() + session: SQLAlchemySession = ispyb_db_session_factory(bind=connection) + session.begin_nested() # Save point for test - # Let other function run - yield ispyb_db_session + # Attach the listener to the session for this connection + attach_event_listener(session) - # Tidy up after function is complete - ispyb_db_session.rollback() - ispyb_db_session.close() + try: + yield session + finally: + session.close() + transaction.rollback() + connection.close() """ From ee0438678ca40591fad12eebf3e4bcea79cdb2f2 Mon Sep 17 00:00:00 2001 From: Eu Pin Tien Date: Wed, 7 May 2025 16:39:03 +0100 Subject: [PATCH 32/47] Rearranged functions and removed 'attach_event_listener()' as a function --- tests/conftest.py | 86 +++++++++++++++++++++++------------------------ 1 file changed, 43 insertions(+), 43 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index 358802022..cdb81bf04 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -77,10 +77,8 @@ def mock_security_configuration( """ ======================================================================================= -Fixtures for setting up mock ISPyB database +Database-related helper functions and classes ======================================================================================= -These were adapted from the tests found at: -https://github.com/DiamondLightSource/ispyb-api/blob/main/tests/conftest.py """ @@ -103,26 +101,6 @@ class ExampleVisit: login = "murfey123" -@pytest.fixture(scope="session") -def ispyb_db_connection(mock_ispyb_credentials): - with ispyb.open(mock_ispyb_credentials) as connection: - yield connection - - -@pytest.fixture(scope="session") -def ispyb_engine(mock_ispyb_credentials): - ispyb_engine = create_engine( - url=url(mock_ispyb_credentials), connect_args={"use_pure": True} - ) - yield ispyb_engine - ispyb_engine.dispose() - - -@pytest.fixture(scope="session") -def ispyb_db_session_factory(ispyb_engine): - return sessionmaker(bind=ispyb_engine, expire_on_commit=False) - - SQLAlchemyTable = TypeVar("SQLAlchemyTable", bound=DeclarativeMeta) @@ -158,6 +136,44 @@ def get_or_create_db_entry( return entry +def restart_savepoint(session: SQLAlchemySession, transaction: RootTransaction): + """ + Re-establish a SAVEPOINT after a nested transaction is committed or rolled back. + This helps to maintain isolation across different test cases. + """ + if transaction.nested and not transaction._parent.nested: + session.begin_nested() + + +""" +======================================================================================= +Fixtures for setting up test ISPyB database +======================================================================================= +These were adapted from the tests found at: +https://github.com/DiamondLightSource/ispyb-api/blob/main/tests/conftest.py +""" + + +@pytest.fixture(scope="session") +def ispyb_db_connection(mock_ispyb_credentials): + with ispyb.open(mock_ispyb_credentials) as connection: + yield connection + + +@pytest.fixture(scope="session") +def ispyb_engine(mock_ispyb_credentials): + engine = create_engine( + url=url(mock_ispyb_credentials), connect_args={"use_pure": True} + ) + yield engine + engine.dispose() + + +@pytest.fixture(scope="session") +def ispyb_db_session_factory(ispyb_engine): + return sessionmaker(bind=ispyb_engine, expire_on_commit=False) + + @pytest.fixture(scope="session") def seed_ispyb_db(ispyb_db_session_factory): @@ -194,22 +210,6 @@ def seed_ispyb_db(ispyb_db_session_factory): ispyb_db_session.close() -def restart_savepoint(session: SQLAlchemySession, transaction: RootTransaction): - """ - Re-establish a SAVEPOINT after a nested transaction is committed or rolled back. - This helps to maintain isolation across different test cases. - """ - if transaction.nested and not transaction._parent.nested: - session.begin_nested() - - -def attach_event_listener(session: SQLAlchemySession): - """ - Attach the restart_savepoint function as an event listener for after_transaction_end - """ - event.listen(session, "after_transaction_end", restart_savepoint) - - @pytest.fixture def ispyb_db_session( ispyb_db_session_factory, @@ -217,7 +217,7 @@ def ispyb_db_session( seed_ispyb_db, ) -> Generator[SQLAlchemySession, None, None]: """ - Returns a test-safe session that wraps each test in a rollback-safe SAVEPOINT. + Returns a test-safe session that wraps each test in a rollback-safe save point. """ connection = ispyb_engine.connect() transaction = connection.begin() # Outer transaction @@ -225,8 +225,8 @@ def ispyb_db_session( session: SQLAlchemySession = ispyb_db_session_factory(bind=connection) session.begin_nested() # Save point for test - # Attach the listener to the session for this connection - attach_event_listener(session) + # Trigger the restart_savepoint function after the end of the transaction + event.listen(session, "after_transaction_end", restart_savepoint) try: yield session @@ -238,7 +238,7 @@ def ispyb_db_session( """ ======================================================================================= -Fixtures for setting up mock Murfey database +Fixtures for setting up test Murfey database ======================================================================================= """ From bd06a435e7c15ee4bf2038989bc81bf896f9d711 Mon Sep 17 00:00:00 2001 From: Eu Pin Tien Date: Wed, 7 May 2025 17:45:46 +0100 Subject: [PATCH 33/47] Replaced 'start_postgres' with same test-safe functions as for ISPyB database --- tests/conftest.py | 64 +++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 54 insertions(+), 10 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index cdb81bf04..f1e9e597c 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,4 +1,5 @@ import json +import os from configparser import ConfigParser from pathlib import Path from typing import Any, Generator, Type, TypeVar @@ -12,8 +13,6 @@ from sqlalchemy.orm import sessionmaker from murfey.util.db import Session as MurfeySession -from murfey.util.db import clear, setup -from tests import murfey_db_engine, murfey_db_url @pytest.fixture(scope="session") @@ -243,12 +242,57 @@ def ispyb_db_session( """ +@pytest.fixture(scope="session") +def murfey_db_engine(): + url = ( + f"postgresql+psycopg2://{os.environ['POSTGRES_USER']}:{os.environ['POSTGRES_PASSWORD']}" + f"@{os.environ['POSTGRES_HOST']}:{os.environ['POSTGRES_PORT']}/{os.environ['POSTGRES_DB']}" + ) + engine = create_engine(url) + yield engine + engine.dispose() + + +@pytest.fixture(scope="session") +def murfey_db_session_factory(murfey_db_engine): + return sessionmaker(bind=murfey_db_engine, expire_on_commit=False) + + +@pytest.fixture(scope="session") +def seed_murfey_db(murfey_db_session_factory): + # Populate Murfey database with initial values + murfey_session: SQLAlchemySession = murfey_db_session_factory() + _ = get_or_create_db_entry( + session=murfey_session, + table=MurfeySession, + lookup_kwargs={ + "name": f"{ExampleVisit.proposal_code}{ExampleVisit.proposal_number}-{ExampleVisit.visit_number}", + }, + ) + murfey_session.close() + + @pytest.fixture -def start_postgres(): - clear(murfey_db_url) - setup(murfey_db_url) - - murfey_session = MurfeySession(id=2, name="cm12345-6") - with SQLAlchemySession(murfey_db_engine) as murfey_db: - murfey_db.add(murfey_session) - murfey_db.commit() +def murfey_db_session( + murfey_db_session_factory, + murfey_db_engine: Engine, + seed_murfey_db, +) -> Generator[SQLAlchemySession, None, None]: + """ + Returns a test-safe session that wraps each test in a rollback-safe save point + """ + connection = murfey_db_engine.connect() + transaction = connection.begin() + + session: SQLAlchemySession = murfey_db_session_factory(bind=connection) + session.begin_nested() # Save point for test + + # Trigger the restart_savepoint function after the end of the transaction + event.listen(session, "after_transaction_end", restart_savepoint) + + try: + yield session + finally: + session.close() + transaction.rollback() + connection.close() From 8f298d8a4af0b340141d07d2be81181c759c6df6 Mon Sep 17 00:00:00 2001 From: Eu Pin Tien Date: Wed, 7 May 2025 17:47:04 +0100 Subject: [PATCH 34/47] Replaced use of 'start_postgres' with test-safe 'murfey_db_session' fixtures --- .../spa/test_flush_spa_preprocess.py | 44 ++++++++----------- 1 file changed, 19 insertions(+), 25 deletions(-) diff --git a/tests/workflows/spa/test_flush_spa_preprocess.py b/tests/workflows/spa/test_flush_spa_preprocess.py index 408100d52..4fcfddbba 100644 --- a/tests/workflows/spa/test_flush_spa_preprocess.py +++ b/tests/workflows/spa/test_flush_spa_preprocess.py @@ -5,11 +5,12 @@ from murfey.util.db import DataCollectionGroup, GridSquare from murfey.util.models import GridSquareParameters from murfey.workflows.spa import flush_spa_preprocess -from tests import murfey_db_engine @mock.patch("murfey.workflows.spa.flush_spa_preprocess._transport_object") -def test_register_grid_square_update_add_locations(mock_transport, start_postgres): +def test_register_grid_square_update_add_locations( + mock_transport, murfey_db_session: Session +): """Test the updating of an existing grid square""" # Create a grid square to update grid_square = GridSquare( @@ -18,9 +19,8 @@ def test_register_grid_square_update_add_locations(mock_transport, start_postgre session_id=2, tag="session_tag", ) - with Session(murfey_db_engine) as murfey_db: - murfey_db.add(grid_square) - murfey_db.commit() + murfey_db_session.add(grid_square) + murfey_db_session.commit() # Parameters to update with new_parameters = GridSquareParameters( @@ -32,15 +32,13 @@ def test_register_grid_square_update_add_locations(mock_transport, start_postgre ) # Run the registration - with Session(murfey_db_engine) as murfey_db: - flush_spa_preprocess.register_grid_square(2, 101, new_parameters, murfey_db) + flush_spa_preprocess.register_grid_square(2, 101, new_parameters, murfey_db_session) # Check this would have updated ispyb mock_transport.do_update_grid_square.assert_called_with(1, new_parameters) # Confirm the database was updated - with Session(murfey_db_engine) as murfey_db: - grid_square_final_parameters = murfey_db.exec(select(GridSquare)).one() + grid_square_final_parameters = murfey_db_session.exec(select(GridSquare)).one() assert grid_square_final_parameters.x_location == new_parameters.x_location assert grid_square_final_parameters.y_location == new_parameters.y_location assert ( @@ -52,7 +50,9 @@ def test_register_grid_square_update_add_locations(mock_transport, start_postgre @mock.patch("murfey.workflows.spa.flush_spa_preprocess._transport_object") -def test_register_grid_square_update_add_nothing(mock_transport, start_postgres): +def test_register_grid_square_update_add_nothing( + mock_transport, murfey_db_session: Session +): """Test the updating of an existing grid square, but with nothing to update with""" # Create a grid square to update grid_square = GridSquare( @@ -65,23 +65,20 @@ def test_register_grid_square_update_add_nothing(mock_transport, start_postgres) x_stage_position=0.3, y_stage_position=0.4, ) - with Session(murfey_db_engine) as murfey_db: - murfey_db.add(grid_square) - murfey_db.commit() + murfey_db_session.add(grid_square) + murfey_db_session.commit() # Parameters to update with new_parameters = GridSquareParameters(tag="session_tag") # Run the registration - with Session(murfey_db_engine) as murfey_db: - flush_spa_preprocess.register_grid_square(2, 101, new_parameters, murfey_db) + flush_spa_preprocess.register_grid_square(2, 101, new_parameters, murfey_db_session) # Check this would have updated ispyb mock_transport.do_update_grid_square.assert_called_with(1, new_parameters) # Confirm the database was not updated - with Session(murfey_db_engine) as murfey_db: - grid_square_final_parameters = murfey_db.exec(select(GridSquare)).one() + grid_square_final_parameters = murfey_db_session.exec(select(GridSquare)).one() assert grid_square_final_parameters.x_location == 0.1 assert grid_square_final_parameters.y_location == 0.2 assert grid_square_final_parameters.x_stage_position == 0.3 @@ -90,7 +87,7 @@ def test_register_grid_square_update_add_nothing(mock_transport, start_postgres) @mock.patch("murfey.workflows.spa.flush_spa_preprocess._transport_object") def test_register_grid_square_insert_with_ispyb( - mock_transport, start_postgres, tmp_path + mock_transport, murfey_db_session, tmp_path ): # Create a data collection group for lookups grid_square = DataCollectionGroup( @@ -99,9 +96,8 @@ def test_register_grid_square_insert_with_ispyb( tag="session_tag", atlas_id=90, ) - with Session(murfey_db_engine) as murfey_db: - murfey_db.add(grid_square) - murfey_db.commit() + murfey_db_session.add(grid_square) + murfey_db_session.commit() # Set the ispyb return mock_transport.do_insert_grid_square.return_value = { @@ -126,15 +122,13 @@ def test_register_grid_square_insert_with_ispyb( ) # Run the registration - with Session(murfey_db_engine) as murfey_db: - flush_spa_preprocess.register_grid_square(2, 101, new_parameters, murfey_db) + flush_spa_preprocess.register_grid_square(2, 101, new_parameters, murfey_db_session) # Check this would have updated ispyb mock_transport.do_insert_grid_square.assert_called_with(90, 101, new_parameters) # Confirm the database entry was made - with Session(murfey_db_engine) as murfey_db: - grid_square_final_parameters = murfey_db.exec(select(GridSquare)).one() + grid_square_final_parameters = murfey_db_session.exec(select(GridSquare)).one() assert grid_square_final_parameters.id == 1 assert grid_square_final_parameters.name == 101 assert grid_square_final_parameters.session_id == 2 From 89a02b96e3f2993792e76687f70c8b9dd45cda8b Mon Sep 17 00:00:00 2001 From: Eu Pin Tien Date: Wed, 7 May 2025 17:55:52 +0100 Subject: [PATCH 35/47] Populate Murfey database tables using 'SQLModel.metadata.create_all()' --- tests/conftest.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/conftest.py b/tests/conftest.py index f1e9e597c..03d740162 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -11,6 +11,7 @@ from sqlalchemy.ext.declarative import DeclarativeMeta from sqlalchemy.orm import Session as SQLAlchemySession from sqlalchemy.orm import sessionmaker +from sqlmodel import SQLModel from murfey.util.db import Session as MurfeySession @@ -249,6 +250,7 @@ def murfey_db_engine(): f"@{os.environ['POSTGRES_HOST']}:{os.environ['POSTGRES_PORT']}/{os.environ['POSTGRES_DB']}" ) engine = create_engine(url) + SQLModel.metadata.create_all(engine) yield engine engine.dispose() From 16fbc416d7263f11ce4d3e47a07ceb15db03adbd Mon Sep 17 00:00:00 2001 From: Eu Pin Tien Date: Wed, 7 May 2025 18:03:37 +0100 Subject: [PATCH 36/47] Murfey's Session table requires a value for 'id'; it doesn't auto-increment --- tests/conftest.py | 4 ++++ tests/workflows/spa/test_flush_spa_preprocess.py | 9 +++++---- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index 03d740162..a7a79810c 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -95,6 +95,9 @@ class ExampleVisit: proposal_number = 12345 visit_number = 6 + # Murfey-specific + murfey_session_id = 1 + # Person (ISPyB) given_name = "Eliza" family_name = "Murfey" @@ -268,6 +271,7 @@ def seed_murfey_db(murfey_db_session_factory): session=murfey_session, table=MurfeySession, lookup_kwargs={ + "session": ExampleVisit.murfey_session_id, "name": f"{ExampleVisit.proposal_code}{ExampleVisit.proposal_number}-{ExampleVisit.visit_number}", }, ) diff --git a/tests/workflows/spa/test_flush_spa_preprocess.py b/tests/workflows/spa/test_flush_spa_preprocess.py index 4fcfddbba..3a233424c 100644 --- a/tests/workflows/spa/test_flush_spa_preprocess.py +++ b/tests/workflows/spa/test_flush_spa_preprocess.py @@ -5,6 +5,7 @@ from murfey.util.db import DataCollectionGroup, GridSquare from murfey.util.models import GridSquareParameters from murfey.workflows.spa import flush_spa_preprocess +from tests.conftest import ExampleVisit @mock.patch("murfey.workflows.spa.flush_spa_preprocess._transport_object") @@ -16,7 +17,7 @@ def test_register_grid_square_update_add_locations( grid_square = GridSquare( id=1, name=101, - session_id=2, + session_id=ExampleVisit.murfey_session_id, tag="session_tag", ) murfey_db_session.add(grid_square) @@ -58,7 +59,7 @@ def test_register_grid_square_update_add_nothing( grid_square = GridSquare( id=1, name=101, - session_id=2, + session_id=ExampleVisit.murfey_session_id, tag="session_tag", x_location=0.1, y_location=0.2, @@ -92,7 +93,7 @@ def test_register_grid_square_insert_with_ispyb( # Create a data collection group for lookups grid_square = DataCollectionGroup( id=1, - session_id=2, + session_id=ExampleVisit.murfey_session_id, tag="session_tag", atlas_id=90, ) @@ -131,7 +132,7 @@ def test_register_grid_square_insert_with_ispyb( grid_square_final_parameters = murfey_db_session.exec(select(GridSquare)).one() assert grid_square_final_parameters.id == 1 assert grid_square_final_parameters.name == 101 - assert grid_square_final_parameters.session_id == 2 + assert grid_square_final_parameters.session_id == ExampleVisit.murfey_session_id assert grid_square_final_parameters.tag == "session_tag" assert grid_square_final_parameters.x_location == 1.1 assert grid_square_final_parameters.y_location == 1.2 From f065efc6ec7d1fda7845724c58e60977feed0655 Mon Sep 17 00:00:00 2001 From: Eu Pin Tien Date: Wed, 7 May 2025 18:07:30 +0100 Subject: [PATCH 37/47] Wrong column name --- tests/conftest.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/conftest.py b/tests/conftest.py index a7a79810c..a4f3aeb07 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -271,7 +271,7 @@ def seed_murfey_db(murfey_db_session_factory): session=murfey_session, table=MurfeySession, lookup_kwargs={ - "session": ExampleVisit.murfey_session_id, + "id": ExampleVisit.murfey_session_id, "name": f"{ExampleVisit.proposal_code}{ExampleVisit.proposal_number}-{ExampleVisit.visit_number}", }, ) From 162288c48574dedeaefbe7a6cd1959abfa925f72 Mon Sep 17 00:00:00 2001 From: Eu Pin Tien Date: Wed, 7 May 2025 18:24:20 +0100 Subject: [PATCH 38/47] Murfey database uses SQLModel commands; recreate Murfey databaes fixtures as SQLModel sessions --- tests/conftest.py | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index a4f3aeb07..d2d9a4e13 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,3 +1,5 @@ +from __future__ import annotations + import json import os from configparser import ConfigParser @@ -11,6 +13,7 @@ from sqlalchemy.ext.declarative import DeclarativeMeta from sqlalchemy.orm import Session as SQLAlchemySession from sqlalchemy.orm import sessionmaker +from sqlmodel import Session as SQLModelSession from sqlmodel import SQLModel from murfey.util.db import Session as MurfeySession @@ -108,7 +111,7 @@ class ExampleVisit: def get_or_create_db_entry( - session: SQLAlchemySession, + session: SQLAlchemySession | SQLModelSession, table: Type[SQLAlchemyTable], lookup_kwargs: dict[str, Any] = {}, insert_kwargs: dict[str, Any] = {}, @@ -139,7 +142,9 @@ def get_or_create_db_entry( return entry -def restart_savepoint(session: SQLAlchemySession, transaction: RootTransaction): +def restart_savepoint( + session: SQLAlchemySession | SQLModelSession, transaction: RootTransaction +): """ Re-establish a SAVEPOINT after a nested transaction is committed or rolled back. This helps to maintain isolation across different test cases. @@ -260,22 +265,24 @@ def murfey_db_engine(): @pytest.fixture(scope="session") def murfey_db_session_factory(murfey_db_engine): - return sessionmaker(bind=murfey_db_engine, expire_on_commit=False) + return sessionmaker( + bind=murfey_db_engine, expire_on_commit=False, class_=SQLModelSession + ) @pytest.fixture(scope="session") def seed_murfey_db(murfey_db_session_factory): # Populate Murfey database with initial values - murfey_session: SQLAlchemySession = murfey_db_session_factory() + session: SQLModelSession = murfey_db_session_factory() _ = get_or_create_db_entry( - session=murfey_session, + session=session, table=MurfeySession, lookup_kwargs={ "id": ExampleVisit.murfey_session_id, "name": f"{ExampleVisit.proposal_code}{ExampleVisit.proposal_number}-{ExampleVisit.visit_number}", }, ) - murfey_session.close() + session.close() @pytest.fixture @@ -283,14 +290,14 @@ def murfey_db_session( murfey_db_session_factory, murfey_db_engine: Engine, seed_murfey_db, -) -> Generator[SQLAlchemySession, None, None]: +) -> Generator[SQLModelSession, None, None]: """ Returns a test-safe session that wraps each test in a rollback-safe save point """ connection = murfey_db_engine.connect() transaction = connection.begin() - session: SQLAlchemySession = murfey_db_session_factory(bind=connection) + session: SQLModelSession = murfey_db_session_factory(bind=connection) session.begin_nested() # Save point for test # Trigger the restart_savepoint function after the end of the transaction From 8c37fde4a6e915e0be185a9acafd108670ffa541 Mon Sep 17 00:00:00 2001 From: Eu Pin Tien Date: Wed, 7 May 2025 18:25:09 +0100 Subject: [PATCH 39/47] Forgot a type hint --- tests/workflows/spa/test_flush_spa_preprocess.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/workflows/spa/test_flush_spa_preprocess.py b/tests/workflows/spa/test_flush_spa_preprocess.py index 3a233424c..581a8e92a 100644 --- a/tests/workflows/spa/test_flush_spa_preprocess.py +++ b/tests/workflows/spa/test_flush_spa_preprocess.py @@ -88,7 +88,7 @@ def test_register_grid_square_update_add_nothing( @mock.patch("murfey.workflows.spa.flush_spa_preprocess._transport_object") def test_register_grid_square_insert_with_ispyb( - mock_transport, murfey_db_session, tmp_path + mock_transport, murfey_db_session: Session, tmp_path ): # Create a data collection group for lookups grid_square = DataCollectionGroup( From 8e1e9205d6c96b2d1eefaa68551e27b8fc299c6e Mon Sep 17 00:00:00 2001 From: Eu Pin Tien Date: Wed, 7 May 2025 18:31:08 +0100 Subject: [PATCH 40/47] Missed replacing some Murfey session ID parameters --- tests/workflows/spa/test_flush_spa_preprocess.py | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/tests/workflows/spa/test_flush_spa_preprocess.py b/tests/workflows/spa/test_flush_spa_preprocess.py index 581a8e92a..04c03a786 100644 --- a/tests/workflows/spa/test_flush_spa_preprocess.py +++ b/tests/workflows/spa/test_flush_spa_preprocess.py @@ -33,7 +33,9 @@ def test_register_grid_square_update_add_locations( ) # Run the registration - flush_spa_preprocess.register_grid_square(2, 101, new_parameters, murfey_db_session) + flush_spa_preprocess.register_grid_square( + ExampleVisit.murfey_session_id, 101, new_parameters, murfey_db_session + ) # Check this would have updated ispyb mock_transport.do_update_grid_square.assert_called_with(1, new_parameters) @@ -73,7 +75,9 @@ def test_register_grid_square_update_add_nothing( new_parameters = GridSquareParameters(tag="session_tag") # Run the registration - flush_spa_preprocess.register_grid_square(2, 101, new_parameters, murfey_db_session) + flush_spa_preprocess.register_grid_square( + ExampleVisit.murfey_session_id, 101, new_parameters, murfey_db_session + ) # Check this would have updated ispyb mock_transport.do_update_grid_square.assert_called_with(1, new_parameters) @@ -123,7 +127,9 @@ def test_register_grid_square_insert_with_ispyb( ) # Run the registration - flush_spa_preprocess.register_grid_square(2, 101, new_parameters, murfey_db_session) + flush_spa_preprocess.register_grid_square( + ExampleVisit.murfey_session_id, 101, new_parameters, murfey_db_session + ) # Check this would have updated ispyb mock_transport.do_insert_grid_square.assert_called_with(90, 101, new_parameters) From eb762cdc4d9e4a9f97beab7ddf973ecf932fdaaa Mon Sep 17 00:00:00 2001 From: Eu Pin Tien Date: Thu, 8 May 2025 09:57:27 +0100 Subject: [PATCH 41/47] 'tests/__init__.py' can be emptied --- tests/__init__.py | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/tests/__init__.py b/tests/__init__.py index 1f1407cb1..e69de29bb 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -1,10 +0,0 @@ -import os - -from sqlmodel import create_engine - -murfey_db_url = ( - f"postgresql+psycopg2://{os.environ['POSTGRES_USER']}:{os.environ['POSTGRES_PASSWORD']}" - f"@{os.environ['POSTGRES_HOST']}:{os.environ['POSTGRES_PORT']}/{os.environ['POSTGRES_DB']}" -) - -murfey_db_engine = create_engine(murfey_db_url) From 88b3bee405b16e7e1122535cdbb659957ad426d1 Mon Sep 17 00:00:00 2001 From: Eu Pin Tien Date: Thu, 8 May 2025 10:10:20 +0100 Subject: [PATCH 42/47] Placeholder for future tests --- tests/server/test_ispyb.py | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/tests/server/test_ispyb.py b/tests/server/test_ispyb.py index 29cb8f3c2..66f84201e 100644 --- a/tests/server/test_ispyb.py +++ b/tests/server/test_ispyb.py @@ -1,4 +1,5 @@ from ispyb.sqlalchemy import BLSession, Proposal +from pytest import mark from sqlalchemy import select from sqlalchemy.orm import Session @@ -56,3 +57,18 @@ def test_get_proposal_id( db=ispyb_db_session, ) assert query == result + + +@mark.skip +def test_get_sub_samples_from_visit(): + pass + + +@mark.skip +def test_get_all_ongoing_visits(): + pass + + +@mark.skip +def test_get_data_collection_group_ids(): + pass From 7f84d4ca9d9c0ac3d4a1879a55020b0432672854 Mon Sep 17 00:00:00 2001 From: Eu Pin Tien Date: Thu, 8 May 2025 15:07:49 +0100 Subject: [PATCH 43/47] Added 'ISPyBTableValues' class to store default table values in ISPyB; seed ISPyB database with ExperimentType information as well --- tests/conftest.py | 32 +++++++++++++++++++++++++++++--- 1 file changed, 29 insertions(+), 3 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index d2d9a4e13..c7db199e9 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -8,7 +8,7 @@ import ispyb import pytest -from ispyb.sqlalchemy import BLSession, Person, Proposal, url +from ispyb.sqlalchemy import BLSession, ExperimentType, Person, Proposal, url from sqlalchemy import Engine, RootTransaction, and_, create_engine, event, select from sqlalchemy.ext.declarative import DeclarativeMeta from sqlalchemy.orm import Session as SQLAlchemySession @@ -87,8 +87,8 @@ def mock_security_configuration( class ExampleVisit: """ - This is a class to store information that will common to all database entries for - a particular Murfey session, to enable ease of replication when creating database + This class stores information that will be common to all database entries for a + particular Murfey session, to enable ease of replication when creating database fixtures. """ @@ -107,6 +107,19 @@ class ExampleVisit: login = "murfey123" +class ISPyBTableValues: + """ + Visit-independent default values for ISPyB tables + """ + + # ExperimentType (ISPyB) + experiment_types = { + "Tomography": 36, + "Single Particle": 37, + "Atlas": 44, + } + + SQLAlchemyTable = TypeVar("SQLAlchemyTable", bound=DeclarativeMeta) @@ -215,6 +228,19 @@ def seed_ispyb_db(ispyb_db_session_factory): "visit_number": ExampleVisit.visit_number, }, ) + _ = [ + get_or_create_db_entry( + session=ispyb_db_session, + table=ExperimentType, + lookup_kwargs={ + "experimentTypeId": id, + "name": name, + "proposalType": "em", + "active": 1, + }, + ) + for name, id in ISPyBTableValues.experiment_types.items() + ] ispyb_db_session.close() From 1cd0aabc6b151740f58e696315cddcd4c3ea86cc Mon Sep 17 00:00:00 2001 From: Eu Pin Tien Date: Thu, 8 May 2025 15:15:18 +0100 Subject: [PATCH 44/47] 'execute()' is deprecated for SQLModel Session objects, so use 'exec()' instead --- tests/conftest.py | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index c7db199e9..bb1d86f37 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -140,9 +140,18 @@ def get_or_create_db_entry( conditions = [ getattr(table, key) == value for key, value in lookup_kwargs.items() ] - entry = ( - session.execute(select(table).where(and_(*conditions))).scalars().first() - ) + # Use 'exec()' for SQLModel sessions + if isinstance(session, SQLModelSession): + entry = session.exec(select(table).where(and_(*conditions))).first() + # Use 'execute()' for SQLAlchemy sessions + elif isinstance(session, SQLAlchemySession): + entry = ( + session.execute(select(table).where(and_(*conditions))) + .scalars() + .first() + ) + else: + raise TypeError("Unexpected Session type") if entry: return entry From 06aad022e91da3addd7849d76205b03eb760d2a5 Mon Sep 17 00:00:00 2001 From: Eu Pin Tien Date: Thu, 8 May 2025 15:23:40 +0100 Subject: [PATCH 45/47] Changed variable names; added unit test for 'get_data_collection_group_ids()' --- tests/server/test_ispyb.py | 51 ++++++++++++++++++++++++++++++-------- 1 file changed, 41 insertions(+), 10 deletions(-) diff --git a/tests/server/test_ispyb.py b/tests/server/test_ispyb.py index 66f84201e..2ed29f6e2 100644 --- a/tests/server/test_ispyb.py +++ b/tests/server/test_ispyb.py @@ -1,17 +1,21 @@ -from ispyb.sqlalchemy import BLSession, Proposal +from ispyb.sqlalchemy import BLSession, DataCollectionGroup, Proposal from pytest import mark from sqlalchemy import select from sqlalchemy.orm import Session -from murfey.server.ispyb import get_proposal_id, get_session_id -from tests.conftest import ExampleVisit +from murfey.server.ispyb import ( + get_data_collection_group_ids, + get_proposal_id, + get_session_id, +) +from tests.conftest import ExampleVisit, ISPyBTableValues def test_get_session_id( ispyb_db_session: Session, ): # Manually get the BLSession ID for comparison - query = ( + bl_session_id = ( ispyb_db_session.execute( select(BLSession) .join(Proposal) @@ -33,14 +37,14 @@ def test_get_session_id( visit_number=str(ExampleVisit.visit_number), db=ispyb_db_session, ) - assert query == result + assert bl_session_id == result def test_get_proposal_id( ispyb_db_session: Session, ): # Manually query the Proposal ID - query = ( + proposal_id = ( ispyb_db_session.execute( select(Proposal) .where(Proposal.proposalCode == ExampleVisit.proposal_code) @@ -56,7 +60,7 @@ def test_get_proposal_id( proposal_number=ExampleVisit.proposal_number, db=ispyb_db_session, ) - assert query == result + assert proposal_id == result @mark.skip @@ -69,6 +73,33 @@ def test_get_all_ongoing_visits(): pass -@mark.skip -def test_get_data_collection_group_ids(): - pass +def test_get_data_collection_group_ids( + ispyb_db_session: Session, +): + # Get the BLSession ID from test database + bl_session_id = get_session_id( + microscope=ExampleVisit.instrument_name, + proposal_code=ExampleVisit.proposal_code, + proposal_number=str(ExampleVisit.proposal_number), + visit_number=str(ExampleVisit.visit_number), + db=ispyb_db_session, + ) + + # Add example data collections + dcgs = [ + { + "sessionId": bl_session_id, + "experimentTypeId": ISPyBTableValues.experiment_types.get(name), + } + for name, id in ISPyBTableValues.experiment_types.items() + ] + dcg_entries = [DataCollectionGroup(**dcg) for dcg in dcgs] + for entry in dcg_entries: + ispyb_db_session.add(entry) + ispyb_db_session.commit() + + # Test the function + results = get_data_collection_group_ids( + session_id=bl_session_id, + ) + assert len(results) == len(dcg_entries) From d66c453e280199ca474e63bacc434ea1420cc7c7 Mon Sep 17 00:00:00 2001 From: Eu Pin Tien Date: Thu, 8 May 2025 15:31:23 +0100 Subject: [PATCH 46/47] 'get_data_collection_group_ids' does not appear to be in use; deleted from Murfey --- src/murfey/server/ispyb.py | 13 ----------- tests/server/test_ispyb.py | 46 +++++--------------------------------- 2 files changed, 5 insertions(+), 54 deletions(-) diff --git a/src/murfey/server/ispyb.py b/src/murfey/server/ispyb.py index e6aab1065..c12bc3939 100644 --- a/src/murfey/server/ispyb.py +++ b/src/murfey/server/ispyb.py @@ -667,16 +667,3 @@ def get_all_ongoing_visits(microscope: str, db: Session | None) -> list[Visit]: ) for row in query ] - - -def get_data_collection_group_ids(session_id): - query = ( - ISPyBSession() - .query(DataCollectionGroup) - .filter( - DataCollectionGroup.sessionId == session_id, - ) - .all() - ) - dcgids = [row.dataCollectionGroupId for row in query] - return dcgids diff --git a/tests/server/test_ispyb.py b/tests/server/test_ispyb.py index 2ed29f6e2..d60ada7bc 100644 --- a/tests/server/test_ispyb.py +++ b/tests/server/test_ispyb.py @@ -1,14 +1,10 @@ -from ispyb.sqlalchemy import BLSession, DataCollectionGroup, Proposal +from ispyb.sqlalchemy import BLSession, Proposal from pytest import mark from sqlalchemy import select from sqlalchemy.orm import Session -from murfey.server.ispyb import ( - get_data_collection_group_ids, - get_proposal_id, - get_session_id, -) -from tests.conftest import ExampleVisit, ISPyBTableValues +from murfey.server.ispyb import get_proposal_id, get_session_id +from tests.conftest import ExampleVisit def test_get_session_id( @@ -48,7 +44,7 @@ def test_get_proposal_id( ispyb_db_session.execute( select(Proposal) .where(Proposal.proposalCode == ExampleVisit.proposal_code) - .where(Proposal.proposalNumber == ExampleVisit.proposal_number) + .where(Proposal.proposalNumber == str(ExampleVisit.proposal_number)) ) .scalar_one() .proposalId @@ -57,7 +53,7 @@ def test_get_proposal_id( # Test function result = get_proposal_id( proposal_code=ExampleVisit.proposal_code, - proposal_number=ExampleVisit.proposal_number, + proposal_number=str(ExampleVisit.proposal_number), db=ispyb_db_session, ) assert proposal_id == result @@ -71,35 +67,3 @@ def test_get_sub_samples_from_visit(): @mark.skip def test_get_all_ongoing_visits(): pass - - -def test_get_data_collection_group_ids( - ispyb_db_session: Session, -): - # Get the BLSession ID from test database - bl_session_id = get_session_id( - microscope=ExampleVisit.instrument_name, - proposal_code=ExampleVisit.proposal_code, - proposal_number=str(ExampleVisit.proposal_number), - visit_number=str(ExampleVisit.visit_number), - db=ispyb_db_session, - ) - - # Add example data collections - dcgs = [ - { - "sessionId": bl_session_id, - "experimentTypeId": ISPyBTableValues.experiment_types.get(name), - } - for name, id in ISPyBTableValues.experiment_types.items() - ] - dcg_entries = [DataCollectionGroup(**dcg) for dcg in dcgs] - for entry in dcg_entries: - ispyb_db_session.add(entry) - ispyb_db_session.commit() - - # Test the function - results = get_data_collection_group_ids( - session_id=bl_session_id, - ) - assert len(results) == len(dcg_entries) From f0f70f3b5dd981360db446fffe4d68a374c2458f Mon Sep 17 00:00:00 2001 From: Eu Pin Tien Date: Fri, 9 May 2025 14:33:36 +0100 Subject: [PATCH 47/47] Updated functions to use renamed ISPyBSession() variable --- src/murfey/cli/spa_ispyb_messages.py | 4 ++-- src/murfey/server/__init__.py | 10 +++++----- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/murfey/cli/spa_ispyb_messages.py b/src/murfey/cli/spa_ispyb_messages.py index 0cca27dca..87264366d 100644 --- a/src/murfey/cli/spa_ispyb_messages.py +++ b/src/murfey/cli/spa_ispyb_messages.py @@ -19,7 +19,7 @@ from murfey.client.contexts.spa import _get_xml_list_index from murfey.server import _murfey_id, _register -from murfey.server.ispyb import Session, TransportManager, get_session_id +from murfey.server.ispyb import ISPyBSession, TransportManager, get_session_id from murfey.server.murfey_db import url from murfey.util import db from murfey.util.config import get_machine_config, get_microscope, get_security_config @@ -256,7 +256,7 @@ def run(): proposal_code=args.visit[:2], proposal_number=args.visit[2:].split("-")[0], visit_number=args.visit.split("-")[1], - db=Session(), + db=ISPyBSession(), ), ) diff --git a/src/murfey/server/__init__.py b/src/murfey/server/__init__.py index 92dfc44b3..334c5dcbe 100644 --- a/src/murfey/server/__init__.py +++ b/src/murfey/server/__init__.py @@ -46,7 +46,7 @@ import murfey import murfey.server.prometheus as prom import murfey.util.db as db -from murfey.server.ispyb import get_session_id +from murfey.server.ispyb import ISPyBSession, get_session_id from murfey.server.murfey_db import url # murfey_db from murfey.util import LogFilter from murfey.util.config import ( @@ -2203,7 +2203,7 @@ def feedback_callback(header: dict, message: dict) -> None: proposal_code=message["proposal_code"], proposal_number=message["proposal_number"], visit_number=message["visit_number"], - db=murfey.server.ispyb.Session(), + db=ISPyBSession(), ) if dcg_murfey := murfey_db.exec( select(db.DataCollectionGroup) @@ -2281,7 +2281,7 @@ def feedback_callback(header: dict, message: dict) -> None: proposal_code=message["proposal_code"], proposal_number=message["proposal_number"], visit_number=message["visit_number"], - db=murfey.server.ispyb.Session(), + db=ISPyBSession(), ) dcg = murfey_db.exec( select(db.DataCollectionGroup) @@ -2380,7 +2380,7 @@ def feedback_callback(header: dict, message: dict) -> None: ).all(): pid = pj_murfey[0].id else: - if murfey.server.ispyb.Session() is None: + if ISPyBSession() is None: murfey_pj = db.ProcessingJob(recipe=message["recipe"], dc_id=_dcid) else: record = ProcessingJob( @@ -2410,7 +2410,7 @@ def feedback_callback(header: dict, message: dict) -> None: if not murfey_db.exec( select(db.AutoProcProgram).where(db.AutoProcProgram.pj_id == pid) ).all(): - if murfey.server.ispyb.Session() is None: + if ISPyBSession() is None: murfey_app = db.AutoProcProgram(pj_id=pid) else: record = AutoProcProgram(