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

TST: Add integration tests for observe #1208

Merged
merged 6 commits into from
Jun 26, 2020

Conversation

kitchoi
Copy link
Contributor

@kitchoi kitchoi commented Jun 11, 2020

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 with on_trait_change.

Checklist

  • Tests
  • Update API reference (docs/source/traits_api_reference)
  • Update User manual (docs/source/traits_user_manual)
  • Update type annotation hints in traits-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.

@mdickinson
Copy link
Member

Given that these are integration tests rather than unit tests, is there a way that we can avoid depending on mock for these tests?

@kitchoi
Copy link
Contributor Author

kitchoi commented Jun 12, 2020

mock is only used for creating a handler callable that we can track the call count, call arguments, reset the call counts etc very easily. We could replace all that with creating a list, append events to the list, check the length of the list, clear the list, etc. They are to me equivalent in this case and I don't mind either way. For my understanding and for future reference, could you expand a bit more why the dependency is a concern here please?

@kitchoi
Copy link
Contributor Author

kitchoi commented Jun 12, 2020

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.

@mdickinson
Copy link
Member

So mocks are kinda magic in all sorts of ways, and it's conceivable that these tests could pass with mocks but not with ordinary handlers for some reason. If we want these to be integration tests, let's use a real handler.

@kitchoi
Copy link
Contributor Author

kitchoi commented Jun 12, 2020

Before I go change these, is list.append real enough? :)

@kitchoi
Copy link
Contributor Author

kitchoi commented Jun 12, 2020

Unrelated note on using Mock as the callable for the change handler, one could not have been able to use Mock as the change handler for on_trait_change, or use print for on_trait_change because on_trait_change checks the callable signatures, neither Mock and print provide that. observe does not care: if it is a callable and it does not fail when a single argument is given to, it can be used (:duck-typing:)

@mdickinson
Copy link
Member

Before I go change these, is list.append real enough? :)

Yes, of course!

Copy link
Member

@mdickinson mdickinson left a 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.

@kitchoi
Copy link
Contributor Author

kitchoi commented Jun 26, 2020

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.

@mdickinson
Copy link
Member

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.

@kitchoi
Copy link
Contributor Author

kitchoi commented Jun 26, 2020

Sorry for letting this idled for this long. This is ready for review.

@kitchoi kitchoi requested a review from mdickinson June 26, 2020 13:18
Copy link
Member

@mdickinson mdickinson left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you!

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