-
Notifications
You must be signed in to change notification settings - Fork 384
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
Dataframe v2: extensive test suite and associated bug fixes for all existing features #7587
Conversation
c6f6ffe
to
72c3083
Compare
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 wish the semantics part of these tests was more explicit. But maybe that should be kept for end-to-end tests in Python?
(Answering to this and all follow-up comments at once.) We've tried these kinds of things before and they are hell to maintain -- and therefore don't get maintained and rot. If you need a better semantic understanding of what these tests do, you can execute them with
Output
(That is in fact the tool I use to hunt down all these bugs.) |
Also this, yes. These tests are meant to put stress on the corner cases of the spec, not to help end users' grasp the semantics of the API. |
Oh, great. I missed that. (Maybe worth dropping a note about that in the PR and/or test for folks like me?) |
👍 |
Random thought: would be kind of cool to have an option to spawn a viewer loaded with the test data, so the data browser could be used too. |
That one I'll leave up to you 😅 |
// TODO(cmc): at least one basic test for every feature in `QueryExpression2`. | ||
// In no particular order: | ||
// * [x] filtered_index | ||
// * [x] filtered_index_range | ||
// * [x] view_contents | ||
// * [x] selection | ||
// * [ ] filtered_index_values | ||
// * [ ] sampled_index_values | ||
// * [ ] filtered_point_of_view | ||
// * [ ] sparse_fill_strategy | ||
|
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.
One area that we might need to semantically define and test is around timeline column. When does a given timeline show up in the result (assuming selection = None
)?
- If the timeline exists at all in the store?
- If anything at all is logged on it in the context of the view content?
- If it is not empty in the query results?
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.
Good point. I'll add this to the list.
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've mainly looked at the tests, and didn't spot anything outside of what we've discussed 👍🏻
This implements an extensive test suite that makes sure that all existing features of the dataframe API A) have their behavior fully specified in code (so we can at list agree on how things work right now) and B) that said behavior does in fact.. behave.
This unsurprisingly revealed many bugs, which this PR fixes.
The currently existing features are:
Tests will later be added for the following, yet to be implemented features:
Checklist
main
build: rerun.io/viewernightly
build: rerun.io/viewerCHANGELOG.md
and the migration guideTo run all checks from
main
, comment on the PR with@rerun-bot full-check
.