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.

Sign up for GitHub

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

Jump to bottom

Improve tests for @select and @transform #155

Merged
merged 11 commits into from
Aug 24, 2020
Merged

Improve tests for @select and @transform #155

merged 11 commits into from
Aug 24, 2020

Conversation

pdeffebach
Copy link
Collaborator

In preparation of using DataFrames.transform behind the scenes, I am writing a bunch of tests to

  1. Make sure we catch edge cases such as using reserved words
  2. Add @test_throws so we know exactly when we are adding new behavior or fixing current bugs.

Please feel free to add tests as you see fit.

@pdeffebach
Copy link
Collaborator Author

Okay I think this is ready for review.

Remaining tests are

  • transform with a grouped data frame. But the tests for behavior are pretty good and the macro parsing stuff should work the exact same as @transform with a normal data frame
  • @where which I don't have a strategy for replacing with filter but whose tests could still go in this PR
  • @orderby which is like @where. Tests could be added but I don't have much of an idea how behavior will change.
  • @with, for which more tests could be added but whose behavior is at a low risk of changing, since it is just replace_syms! which @transform2 etc. use without any modification.

@bkamins
Copy link
Member

bkamins commented Jul 20, 2020

CI fails

@pdeffebach
Copy link
Collaborator Author

Sorry, @eval woes. It's fixed now, feel free to have a look.

bkamins
bkamins reviewed Jul 20, 2020
View reviewed changes
test/dataframes.jl Outdated
cr = "c"

@testset "@transform" begin
@test @transform(df, n = :i).n == df.i
Copy link
Member

Choose a reason for hiding this comment

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

in several tests I would also check:

  1. if the original df was not changed
  2. if columns got copied (!==) and not just aliased (===)
  3. not only the transformed column but if all other columns are retained

Copy link
Member

Choose a reason for hiding this comment

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

Same for other macros

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

if columns got copied (!==) and not just aliased (===)

It looks like that is not actually the behavior

julia> df = DataFrame(rand(2,2));

julia> @transform(df, n = :x1).n === df.x1
true

Presumably this is not actually behavior we want, so this goes in the limits of testset. Do you agree?

Copy link
Member

Choose a reason for hiding this comment

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

Yes - for sure we want @tranform to make a copy like transform, as this is the expected behavior (so I would put the current tests in the set of tests that are likely to be broken after the update).

In general we might consider adding copycols kwarg to @transform but this can be left for later.

@bkamins
Copy link
Member

bkamins commented Jul 20, 2020

Nice tests - I have tried to check that you have not removed any old tests and it seems so, but just please confirm that this is the case.

@pdeffebach
define Filesystem 1K-blocks Used Available Use% Mounted on
2261568 
udev            16295320         0  16295320   0% /dev
tmpfs            3264564      1916   3262648   1% /run
/dev/sda5      212873684 100856764 101133800  50% /
tmpfs           16322804    838960  15483844   6% /dev/shm
tmpfs               5120         4      5116   1% /run/lock
tmpfs           16322804         0  16322804   0% /sys/fs/cgroup
/dev/loop1          1024      1024         0 100% /snap/gnome-logs/93
/dev/loop2         69248     69248         0 100% /snap/sublime-text/77
/dev/loop3        261760    261760         0 100% /snap/gnome-3-34-1804/33
/dev/loop4         69248     69248         0 100% /snap/sublime-text/85
/dev/loop5        267008    267008         0 100% /snap/kde-frameworks-5-core18/32
/dev/loop8          2560      2560         0 100% /snap/gnome-calculator/748
/dev/loop6           384       384         0 100% /snap/gnome-characters/550
/dev/loop7         56320     56320         0 100% /snap/core18/1880
/dev/loop0        164096    164096         0 100% /snap/gnome-3-28-1804/116
/dev/loop10         6528      6528         0 100% /snap/rclone/466
/dev/loop11       261760    261760         0 100% /snap/gnome-3-34-1804/36
/dev/loop9          2304      2304         0 100% /snap/gnome-system-monitor/148
/dev/loop13        56320     56320         0 100% /snap/core18/1754
/dev/loop12         9344      9344         0 100% /snap/canonical-livepatch/95
/dev/loop14          384       384         0 100% /snap/gnome-characters/539
/dev/loop16        20352     20352         0 100% /snap/okular/90
/dev/loop18        20224     20224         0 100% /snap/okular/98
/dev/loop17       165376    165376         0 100% /snap/gnome-3-28-1804/128
/dev/loop15         2560      2560         0 100% /snap/gnome-calculator/730
/dev/loop19        99328     99328         0 100% /snap/core/9665
/dev/loop20         1024      1024         0 100% /snap/gnome-logs/100
/dev/loop21        98944     98944         0 100% /snap/core/9436
/dev/loop22        63616     63616         0 100% /snap/gtk-common-themes/1506
/dev/loop24         2304      2304         0 100% /snap/gnome-system-monitor/145
/dev/loop23       297600    297600         0 100% /snap/kde-frameworks-5-qt-5-14-core18/4
/dev/loop25       296832    296832         0 100% /snap/kde-frameworks-5-qt-5-14-core18/3
/dev/loop27         9344      9344         0 100% /snap/canonical-livepatch/94
/dev/loop26        56192     56192         0 100% /snap/gtk-common-themes/1502
/dev/sda2          98304     33475     64829  35% /boot/efi
tmpfs            3264560       124   3264436   1% /run/user/1000 in local scope
@pdeffebach
add === tests
26fd3c9
@pdeffebach
Copy link
Collaborator Author

Okay i've pinned down some === behavior for @transform and @select.

I can confirm that I didnt delete any tests. Just moved a lot of things.

This was referenced Jul 23, 2020
Merged
Closed
@pdeffebach
Copy link
Collaborator Author

Bumping this. Would be great to get this merged so we can make real changes to @transform

@bkamins
Copy link
Member

bkamins commented Aug 20, 2020

Sure - but can you please resolve merge conflicts first and make sure all tests are correct against current master (without it I am not able to merge this). Thank you!

@bkamins
Copy link
Member

bkamins commented Aug 21, 2020

Looks good to me.
@pdeffebach - when the CI passes and you are happy to merge please ping me and I will merge.

@pdeffebach
Copy link
Collaborator Author

Tests fail. I will look into this today.

@pdeffebach
Copy link
Collaborator Author

Tests pass! Ready for merging

@bkamins bkamins merged commit 6d49937 into JuliaData:master Aug 24, 2020
@bkamins
Copy link
Member

bkamins commented Aug 24, 2020

Thank you!

@pdeffebach
Copy link
Collaborator Author

Now the real work begins!

@pdeffebach pdeffebach mentioned this pull request Aug 31, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bkamins bkamins bkamins approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@pdeffebach @bkamins