BDMS 22+: implement sample model | some pytest cleanup | pre-commit hooks by jacob-a-brown · Pull Request #49 · DataIntegrationGroup/OcotilloAPI

jacob-a-brown · 2025-07-31T23:00:46Z

Why

This PR addresses the following problem / context:

It implements the current version of the sample model, with some anticipated changes coming down the road
Updates pytest session fixtures to be in tests/conftest.py for repeated use and retrieval throughout the testing suite.
Configures pre-commit hooks

How

Implementation summary - the following was changed / added / removed:

get, post, and patch endpoints were made for the /sample router
tests were made for all endpoints, as well as retrieving a single sample by its id
pytest fixtures were moved to tests/conftest.py for use throughout the testing suites
pre-commit hooks were configured in .pre-commit-config.yaml

Notes

Any special considerations, workarounds, or follow-up work to note?

The sample model is still being developed, but the work done here is applicable to future development so I wanted to get it into pre-production (the fixtures in particular)
Notes about alembic migrations were taken in the initial migration script for when alembic and its migrations are adopted
refactoring notes were taken and are found by searching for REFACTOR NOTE/TODO for future ideas/work
Some work was done to make tests independent of each other, but that should be a task for another PR. It was only implemented a bit here.

This commit drops the `idx_location_point` index from the `location` table. The index was causing issues with the database schema because it is automatically created by SQLAlchemy when the `location` table is defined.

…ampleLocations into jab-poc

configure_mappers() to ensure all models are registered and ready for migration.

…ampleLocations into jab-poc

This is to be used by all endpoints where a resource is not found when an id is given as part of a path parameter

The id changes because of autoincrement in the database, so use the fixture's sample id instead of a hardcoded value.

This note has been addressed in another development branch and is no longer relevant in this context.

fixtures were moved to conftest.py to allow for better organization and reuse across multiple test files. This change helps in maintaining cleaner test code and improves the overall structure of the test suite.

Tests that post and patch data need to be undone for fixtures that are used in the full session

jirhiker · 2025-07-31T23:10:54Z

Alembic migrations do not need to be used for the tests. This seems to be a continuing source of complexity and is not necessary. These unit tests are not for testing database migrations, they are for testing that the API functions correctly.

jacob-a-brown · 2025-07-31T23:16:52Z

That makes sense that these are API tests. Would we have a database testing suite, though, to ensure that it's all built and working as expected?

jirhiker

The sample api looks good and the use of fixtures in the tests is an improvement

But I do not like the use of alembic in the tests. It seems unnecessary and adds complexity that is a distraction from the true purpose of the tests

These tests are for the API, not for the database, so no need to use Alembic migrations

jacob-a-brown · 2025-07-31T23:22:32Z

I'm now using Base.metadata.drop_all(engine) and Base.metadata.create_all(engine) for the tests since these are just API, not database, tests.

Use this instead of session: Depends(get_db_session) in POST /sample endpoint to keep a consistent style.

jacob-a-brown and others added 30 commits July 24, 2025 15:13

fix: import all models in db package for Alembic

69c0a9e

fix: drop idx_location_point index

6bece1b

This commit drops the `idx_location_point` index from the `location` table. The index was causing issues with the database schema because it is automatically created by SQLAlchemy when the `location` table is defined.

Formatting changes

a406054

feat: Refactor imports and update Alembic migration for all models

c348dc6

fix: version work in docker database

56e5ade

Merge branch 'jab-poc' of https://github.com/DataIntegrationGroup/NMS…

4920e36

…ampleLocations into jab-poc

Formatting changes

6b3e83b

Merge remote-tracking branch 'origin/pre-production' into jab-poc

0f19295

fix: create migration script with sa continuum tables

70c211c

configure_mappers() to ensure all models are registered and ready for migration.

fix: drop indexes before creation

f4c1b53

Merge branch 'jab-poc' of https://github.com/DataIntegrationGroup/NMS…

af59537

…ampleLocations into jab-poc

Formatting changes

2e6b162

fix: drop index before creating if exists

d471487

fix: remove material created by tests

5c111df

Merge branch 'jab-poc' of https://github.com/DataIntegrationGroup/NMS…

7055f2f

…ampleLocations into jab-poc

fix: resolve merge conflicts

12d993f

docs: document work needed to be done by alembic migrations

3ce1bee

refactor: call configure_mappers() in db/__init__.py

5b223bc

feat: add sample fixture for sample tests

d43365d

fix: delete thing after creation in fixture

56a6229

feat: create general ResourceNotFoundResponse model for 404 errors

5fc4016

This is to be used by all endpoints where a resource is not found when an id is given as part of a path parameter

feat: create sample patch and get by id endpoints

ded3c54

feat: add update schema for sample and update sample response schema

7ef271f

WIP: work on sample endpoint tests

2d40298

fix: implement successful and 404 sample patch endpoints

706006f

WIP: test sample custom field validators

e22f7e8

docs: note where code can be refactored

9f58da3

docs: note fixture management

315f407

fix: update add sample test to use thing fixture

f892b3c

fix: use sample fixture's in get get sample test

e3b127d

The id changes because of autoincrement in the database, so use the fixture's sample id instead of a hardcoded value.

jacob-a-brown added 16 commits July 30, 2025 12:04

fix: fix use of session to validate Thing existence for UpdateSample

d54d5ce

fix: fix errors with 422 custom validation update sample tests

b301103

fix: cleanup sample after add test

7e73e47

refactor: remove outdated note about fixtures

fed04a7

This note has been addressed in another development branch and is no longer relevant in this context.

refactor: put sample fixture in tests/__init__.py for organization

a35b9d1

Merge remote-tracking branch 'origin/pre-production' into dev-jab

acb07dd

fix: fix artifact from merge conflict

d484f2c

refactor: move fixtures to conftest.py

b14a536

fixtures were moved to conftest.py to allow for better organization and reuse across multiple test files. This change helps in maintaining cleaner test code and improves the overall structure of the test suite.

fix: update and clean samples tests for session fixtures

1fe76a1

Tests that post and patch data need to be undone for fixtures that are used in the full session

fix: remove debugging print statement

20b4a12

fix: remove fixture imports now they are in conftest

3ba7dfa

fix: fix search test cases

2d08c74

fix: delete shapefile files created by geospatial test

8406c8c

fix: use session fixtures for contact tests | cleanup post tests

21e6c9a

fix: run search test on relevant fixtures

eb0d1fa

fix: temporary fix until all fixtures are used

6aa3e8f

jacob-a-brown requested a review from jirhiker July 31, 2025 23:00

jirhiker requested changes Jul 31, 2025

View reviewed changes

Comment thread api/sample.py

refactor: drop and create all tables for API testing suite

717dde8

These tests are for the API, not for the database, so no need to use Alembic migrations

refactor: address PR 49 feedback and use session_dependency

57b937a

Use this instead of session: Depends(get_db_session) in POST /sample endpoint to keep a consistent style.

jirhiker approved these changes Aug 1, 2025

View reviewed changes

jirhiker merged commit d7225d7 into pre-production Aug 1, 2025
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BDMS 22+: implement sample model | some pytest cleanup | pre-commit hooks#49

BDMS 22+: implement sample model | some pytest cleanup | pre-commit hooks#49
jirhiker merged 48 commits into
pre-productionfrom
dev-jab

jacob-a-brown commented Jul 31, 2025

Uh oh!

jirhiker commented Jul 31, 2025

Uh oh!

jacob-a-brown commented Jul 31, 2025

Uh oh!

jirhiker left a comment

Uh oh!

Uh oh!

jacob-a-brown commented Jul 31, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

jacob-a-brown commented Jul 31, 2025

Why

How

Notes

Uh oh!

jirhiker commented Jul 31, 2025

Uh oh!

jacob-a-brown commented Jul 31, 2025

Uh oh!

jirhiker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jacob-a-brown commented Jul 31, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants