-
Notifications
You must be signed in to change notification settings - Fork 85
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: Add integration tests for observe #1208
Conversation
Given that these are integration tests rather than unit tests, is there a way that we can avoid depending on |
|
Maybe it is down to what we define as integration tests... I could rename the title if needed. In real-life projects, the change handlers are unlikely to be mock objects, but the objectives in these tests do not reach as far as what happens after the change handlers are called. |
So |
Before I go change these, is |
Unrelated note on using |
Yes, of course! |
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.
Just noting that this PR is awaiting changes, not review.
Thanks for the reminder! Been doing this in my spare cycles, and are also a bit scarce... I will try to get back to this. |
Yep, not urgent: I only submitted the review because I find it helpful to be able to look at the list of PRs in GitHub and see at a glance which ones are awaiting review, which ones are awaiting changes, etc. Maybe I should be setting up a project instead. |
Sorry for letting this idled for this long. This is ready for review. |
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.
LGTM. Thank you!
While the
observation
package is well covered by tests, the tests there are very focused, too focused that they could be a bit out-of-context. This PR adds tests that demonstrate and protect behaviour by testing interactions among different observers. Some of these testes are motivated by existing issues withon_trait_change
.Checklist
Update API reference (docs/source/traits_api_reference
)Update User manual (docs/source/traits_user_manual
)Update type annotation hints intraits-stubs
FTR: These tests have been sitting in a separate branch (in an uglier form) from which PRs for individual components were extracted from for building up
observe
.