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

Add more tests to Interactive and some comments #812

Merged
merged 15 commits into from
Aug 3, 2022

Conversation

maximlt
Copy link
Member

@maximlt maximlt commented Aug 1, 2022

This PR adds various tests to Interactive. The aim of these tests is two-fold: cover parts that were untested and better track the underlying logic of Interactive. More comments in particular are meant to be added.

Interestingly I wrote these tests first based on the 0.7.3 version of hvPlot, before the support for functions as input was added. The logic at the time was slightly easier to comprehend. When writing the tests I added some utility to inspect calls to the _clone method at it seemed central to the way Interactive operates. When I finally ran the tests against master I had to update these tests, _clone is called more than it used too. The logic of Interactive has become more difficult to track.

@maximlt maximlt marked this pull request as draft August 1, 2022 21:38
@maximlt
Copy link
Member Author

maximlt commented Aug 2, 2022

Ah I realized that part of the PR that implemented the support for functions as input to Interactive some other improvements and fixes were made (#720 (comment)), in particular:

Accessors now always return a new interactive object, previously it would act by changing the object inplace resulting in a lot of oddness when you chained that object

Which I believe is the one that led to an increase of calls to _clone, but also obviously allowed to make Interactive more robust!

@maximlt maximlt marked this pull request as ready for review August 3, 2022 10:24
@maximlt
Copy link
Member Author

maximlt commented Aug 3, 2022

This PR doesn't touch any code of interactive.py, it simply adds comments and tests so I will merge as soon as the CI goes green. The tests really on some rather brittle features, it's spying the _clone method to track what Interactive does and it's checking that the dim transforms are correct by using their repr, which isn't guaranteed to be stable and I'm pretty sure I've spotted a few bugs.

Anyway this is better than before, and it has allowed me to better understand how Interactive works, to spot a few obvious bugs I'll fix in following PRs, and to be a little bit more confident when modifying Interactive in the future.

@maximlt maximlt merged commit bfa9310 into master Aug 3, 2022
@maximlt maximlt deleted the interactive_tests_comments branch August 3, 2022 12:10
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.

1 participant