-
Notifications
You must be signed in to change notification settings - Fork 7
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 tests on filter with datetime variables #455
Conversation
integration/test_dataset.py
Outdated
pass | ||
|
||
ds.delete() | ||
ds_to_append.delete() |
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.
Can you assert that the new rows are available after the append?
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 think I added the test, but the behavior is not so easy to understand. In fact, when I fetch the dataset after the append call, there are multiple null values, and the only ones that are not null are the one that are correctly filtered.
Looking at the test and the result in superadmin, I have this in the variable summary:
{
"count": 7,
"min": "2024-04-03T20:00:52.999",
"max": "2024-06-03T20:00:52.123",
"valid_count": 2,
"missing_count": 5,
"missing_frequencies": [
{
"value": {
"?": -1
},
"count": 5
}
],
So, in the test I'm creating a new variable with 5 values. then, I append it and in the resulting dataset I have 7 values: 5 are non valid and 2 valid. I'm not expecting this from this api, but maybe this is the expected behavior
@jjdelc I'm also having issue installing the project locally, because the pycrunch dependency is 0.5.5 from the setup.py file while in the requirements it will get the latest one. Do we need to update the pycrunch version also from the setup.py file? |
Also, I made some changes on the github action file to have a green CI for py2.7, I see now that there is already a PR for this https://github.com/Crunch-io/scrunch/pull/445/files. Let me know if you want me to rebase from that one |
@jjdelc I run the integration tests against alpha which has the code from the fix https://github.com/Crunch-io/zoom/pull/8913 |
Following slack conversation: https://crunch-io.slack.com/archives/C072AKYD58X/p1717521227887949