Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
TST: Implement test IOC #34
TST: Implement test IOC #34
Changes from all commits
7cda787
c526e90
6166def
f289aee
5f5d5bc
f06d2e5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the record, I think this is fine and no changes need to be made. But I do want to clarify:
Was this changed to provide assurance that we couldn't accidentally clobber the global SHIMS? I think my intent was for there to be only one shim of each type per session, on the off chance we started to track running coroutines or caching values. I didn't implement any of that, so there's no reason to revert this right now.
I also thought the way you fixed this in the test suite would have been sufficient, but better safe than sorry haha
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is redundant for fixing the issue, but I figured it would help avoid similar mistakes in the future. If we want to configure shims for a session, as you say, we can revert this line later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason not to make this a fixture as well? I suppose if I ever find myself wanting to use it as a fixture we could add the decorator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Marking it as a fixture makes it difficult to use outside of a
pytest
context, which I ran into when trying to run the test IOC. You can still import and call it in other fixtures, it just won't happen automatically. If we do want fixture controls, I suppose we could make a fixture that simply calls and returnslinac_data
to get the benefits of both options.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've written this function a couple of times at this point (or at least similar looking ones), we should find a good way to DRY this out (later, not this PR)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. As a note for the future PR, the reason I didn't do it here is because it requires untangling backend calls in the client version, which are required for resolving UUIDs.