DM-45816 Yet even more unit tests +947-9 #35
Conversation
| engine = setup_postgres() | ||
| asyncio.run(main()) | ||
| if len(sys.argv) > 1: | ||
| process_local_path(sys.argv[1]) |
There was a problem hiding this comment.
So if this is called with an argument, then the service does not run as a server but just the once, is that correct?
There was a problem hiding this comment.
Yes, that's correct.
There was a problem hiding this comment.
Does it make sense to process a list of files/directories, rather than just one?
| detector INTEGER NOT NULL, | ||
| s_region VARCHAR(1024), | ||
| PRIMARY KEY (ccdexposure_id), | ||
| CONSTRAINT un_exposure_id_detector UNIQUE (exposure_id, detector), |
There was a problem hiding this comment.
I think the CONSTRAINT names need to have the table name in them, in this case ccdexposure
There was a problem hiding this comment.
Don't blame me, blame felis. I always hate to edit machine generated code.
There was a problem hiding this comment.
You can fix felis and in theory build this dynamically from the felis model.
tests/test_pqserver.py
Outdated
| # and with fake made up flex data | ||
| with sqlite3.connect(schema_path) as conn: | ||
| conn.executescript(sql_path.read_text()) | ||
| import time |
There was a problem hiding this comment.
Will you move this import to the top of the module?
There was a problem hiding this comment.
Probably shouldn't be there at all.
There was a problem hiding this comment.
you use time.sleep() two lines below this. Don't forget to remove that too, if that is your intention.
There was a problem hiding this comment.
(Not required) We could move the fixtures out into their own file at this point of complexity. I have used conftest.py in the past, but if these fixtures are only used in this single file it would not make sense to put them there or modularize so much, without the test file also being broken out.
Co-authored-by: Valerie <[email protected]>
Dockerfile.pytest
Outdated
| @@ -2,7 +2,7 @@ ARG OBS_LSST_VERSION=w_2024_21 | |||
| FROM lsstsqre/centos:7-stack-lsst_distrib-${OBS_LSST_VERSION} | |||
| USER lsst | |||
| RUN source loadLSST.bash && mamba install aiokafka httpx | |||
| RUN source loadLSST.bash && pip install kafkit aiokafka httpx pytest-asyncio | |||
| RUN source loadLSST.bash && pip install kafkit aiokafka httpx pytest-asyncio testing.postgresql | |||
There was a problem hiding this comment.
It would be cleaner if we could separate the testing and running environments. It's not a huge deal at present, but it could become so later, and it would be a good pattern to set.
You shouldn't need to mamba install and pip install the same packages. While generally mamba/conda install is slightly safer than pip install for Conda environments, the testing.postgresql package is not available in conda-forge (though pytest-postgresql, which may be similar, is), so using pip for everything makes some sense.
| engine = setup_postgres() | ||
| asyncio.run(main()) | ||
| if len(sys.argv) > 1: | ||
| process_local_path(sys.argv[1]) |
There was a problem hiding this comment.
Does it make sense to process a list of files/directories, rather than just one?
| json={"obs_dict": data}, | ||
| ) | ||
| _assert_http_status(response, 200) | ||
| assert response.json()["obs_id"] == list(data.keys()) |
There was a problem hiding this comment.
Is the ordering guaranteed? Maybe sort to make sure?
Might be nice to do a query to make sure that the data actually got updated.
| ] | ||
| tables = [f"cdb_lsstcomcamsim.{t}" for t in tables] | ||
| for t in tables: | ||
| assert t in result |
There was a problem hiding this comment.
This allows extra tables to be in the schema. Is that wise? Otherwise set equality might be better?
This PR contains the following:
And as an added bonus, functionality in hinfo to load data into the database from local disk storage.