-
Notifications
You must be signed in to change notification settings - Fork 123
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
Implement Property Tests for DataFrame.new
#1012
Implement Property Tests for DataFrame.new
#1012
Conversation
48a1e3d
to
ae63e45
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 really like this PR, thanks for jumping on it!
Sorry I know it's still in draft, but I've added a few suggestions. Also please note that we have our own home-grown Explorer.Duration
struct (until we require 1.17), so Duration.new!
won't work.
Co-authored-by: Billy Lanchantin <william.lanchantin@cargosense.com>
f50b66b
to
f6c1ca0
Compare
@billylanchantin Thanks for the feedback. All should be fixed / applied. I have marked this PR as draft since I have no intention of fixing the bugs it uncovers and also not to define more meaningful tests based on it. (somebody more familiar with the internals will do a lot better job at that) I'll however happily get the generator ready so that someone else can use it to root out all the bugs. |
@maennchen The plan of keeping this as a draft for now makes sense to me. Related: above, you said we (the team) could take over the PR and start making tests based on what we find. Would you mind if I do that? I've made some tweaks to the generator locally and I've already found out a few things. Findings so far:
Mind if I push my changes? |
@billylanchantin Feel free to take over 😊 We should probably also property test the data export formats. #1011 manifests for me both when inspecting as well as when storing as a json or parquet file. |
The existing property test used a special case of the new generator logic. This replaces that special case with the new generator.
We should be able to serialize any DataFrame or document the cases where we can't. None of these are working at the moment.
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.
The team has decided that we'll try to merge a version of this PR with skipped properties. Then we'll try to address the various issues later. We think this approach will be easier to maintain.
I'll also create an issue to track the problems that these property tests have uncovered.
Btw, I think we should have property based testing disabled altogether by default, and we enable it on CI only. There is no need to spend CPU cycles on properties for most of the tests runs that the core team and contributors do. They are very valuable though on CI. |
@josevalim I agree. I actually thought it was already like that but it's not working. Will fix. |
Before we weren't actually skipping property tests. We were excluding the`property`tag, but that tag wasn't actually being set.
Turns out we _were_ setting the `property` tag.
@josevalim All is good. Turns out we were excluding property tests outside of CI all along. Related: TIL that if you use the |
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!
Follow up from #1011 (comment)
Property Tests for
DataFrame.new
seem to be helpful. I already found a panic with it and lots of other issues.This PR is not finished / ready to merge. I just wanted to give it a go.
Feel free to:
Example uncovered issue: