Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DM-32682: Mock out metadata setting in test #142

Merged
merged 1 commit into from Dec 6, 2021
Merged

DM-32682: Mock out metadata setting in test #142

merged 1 commit into from Dec 6, 2021

Conversation

timj
Copy link
Member

@timj timj commented Dec 2, 2021

With the task mocked out the metadata values calculated
by the task are no longer real values but are themselves
MagicMock objects. MagicMock objects can't be stored in
a TaskMetadata so we need to mock out the API that
sets the metadata.

This works with PropertySet since that seems to accept
the value and silently ignores it.

@timj timj requested a review from ktlim December 2, 2021 22:45
tests/test_diaPipe.py Outdated Show resolved Hide resolved
tests/test_diaPipe.py Outdated Show resolved Hide resolved
With the task mocked out the metadata values calculated
by the task are no longer real values but are themselves
MagicMock objects. MagicMock objects can't be stored in
a TaskMetadata so instead the run methods for the
associator subtasks are explicitly mocked to return
usable data for the metadata.

This works with PropertySet since that seems to accept
the mock value and silently ignores it.
@timj
Copy link
Member Author

timj commented Dec 3, 2021

@ktlim I took another look and realized that if I mocked out the run() method of the subtask and not the entire subtask, then I could make the test store valid metadata.

@timj timj requested a review from ktlim December 3, 2021 23:56
with patch.multiple(
task, **{task: DEFAULT for task in subtasksToMock + ["apdb"]}
):
with patch('lsst.ap.association.diaPipe.pd.concat',
new=concatMock):
with patch('lsst.ap.association.diaPipe.pd.concat', new=concatMock), \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe you can parenthesize this whole thing, eliminating the need for backslashes. And can you add in the patch.multiple() as well?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, sorry, the parenthesized list may be Python 3.9.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually Python 3.10.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't merge it with the patch.multiple because I didn't want to change the indent level of the code I wasn't touching.

@timj timj merged commit a900152 into main Dec 6, 2021
@timj timj deleted the tickets/DM-32682 branch December 6, 2021 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants