fix(test): update radionuclide test to stop using thing_id and to validate the real FK/relationship on chemistry_sample_info_id#472
Conversation
…real FK/relationship on `chemistry_sample_info_id`
There was a problem hiding this comment.
Pull request overview
Updates legacy radionuclides tests to align with the actual schema/ORM relationships by removing thing_id expectations and validating the real FK/relationship via chemistry_sample_info_id.
Changes:
- Removed
thing_idusage fromNMA_Radionuclidestest record construction and expected column lists - Updated FK cascade assertion to target
chemistry_sample_info_id - Adjusted relationship test to validate navigation through
chemistry_sample_info
ksmuczynski
changed the title
thing_id and to validate the real FK/relationship on chemistry_sample_info_idthing_id and to validate the real FK/relationship on chemistry_sample_info_id
- verify sample_info.radionuclides includes the new record - keep FK/relationship tests aligned to chemistry_sample_info_id
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
tests/features/steps/nma-legacy-relationships.py:114
- The step function name
step_then_find_by_locationidis now inconsistent with the updated model semantics in this PR (chemistry samples FK toThing, andLocationimport was removed). Consider renaming the step/function to reflectThing/well-based lookup (or reintroduce/retainLocationusage/import if location-based lookup is still required) to avoid confusion and mismatched intent.
def step_then_find_by_locationid(context: Context):
| from sqlalchemy.exc import IntegrityError, StatementError | ||
|
|
||
| from db import Location, Thing | ||
| from db import Thing |
There was a problem hiding this comment.
The step function name step_then_find_by_locationid is now inconsistent with the updated model semantics in this PR (chemistry samples FK to Thing, and Location import was removed). Consider renaming the step/function to reflect Thing/well-based lookup (or reintroduce/retain Location usage/import if location-based lookup is still required) to avoid confusion and mismatched intent.
There was a problem hiding this comment.
I think the step function step_then_find_by_locationid still remains true - a well should be successfully queried by its legacy LocationID (Thing.nma_pk_location). I don't think this needs to be renamed.
| """ | ||
| with session_ctx() as session: | ||
| with pytest.raises(ValueError, match="requires a parent Thing"): | ||
| with pytest.raises(ValueError, match="requires a chemistry_sample_info_id"): |
There was a problem hiding this comment.
This assertion matches a very specific error message string, which can make the test brittle (e.g., small wording changes will break it). Consider loosening the match to a more stable regex that still proves intent (for example, matching on a key phrase like chemistry_sample_info rather than the full sentence).
| with pytest.raises(ValueError, match="requires a chemistry_sample_info_id"): | |
| with pytest.raises(ValueError, match=r"chemistry_sample_info_id"): |
|
|
||
| def test_radionuclides_back_populates_thing(water_well_thing): | ||
| """NMA_Radionuclides.thing navigates back to Thing.""" | ||
| def test_radionuclides_back_populates_sample_info(water_well_thing): |
There was a problem hiding this comment.
Within an explicit session scope, mixing a fixture instance (water_well_thing) with newly persisted objects can be harder to reason about (especially if the fixture instance is detached from the current session). Consider using the session-bound Thing instance (e.g., merge/query inside session_ctx) consistently when wiring thing_id, to keep everything in the same session identity map.
| nma_sample_pt_id=uuid4(), | ||
| nma_sample_point_id=_next_sample_point_id(), | ||
| thing_id=well.id, | ||
| thing_id=water_well_thing.id, |
There was a problem hiding this comment.
Within an explicit session scope, mixing a fixture instance (water_well_thing) with newly persisted objects can be harder to reason about (especially if the fixture instance is detached from the current session). Consider using the session-bound Thing instance (e.g., merge/query inside session_ctx) consistently when wiring thing_id, to keep everything in the same session identity map.
- Moved radionuclide sample-info requirement into its own class - Moved sample-info navigation test under a new navigation section
Why
This PR addresses the following problem / context:
thing_id, but the tests expected it, causing failures.chemistry_sample_info_id.How
Implementation summary - the following was changed / added / removed:
thing_idfrom NMA_Radionuclides test constructions and expected columns.chemistry_sample_info_id.chemistry_sample_info back-population.Notes
Any special considerations, workarounds, or follow-up work to note?
thing_id#471.test_radionuclides_legacy.pypasses.