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

163 more tests for se filtered states #184

Merged
merged 28 commits into from
Sep 10, 2021

Conversation

denisovan31415
Copy link
Contributor

closes #163

@denisovan31415 denisovan31415 marked this pull request as draft August 31, 2021 17:30
@denisovan31415 denisovan31415 marked this pull request as ready for review September 2, 2021 15:14
Copy link
Contributor

@kpagacz kpagacz left a comment

Choose a reason for hiding this comment

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

What do you think about adding tests like this one?

testthat::test_that("get_call returns a call filtering a data.frame based on a ChoicesFilterState", {
choices_dataset <- as.data.frame(list(choices = c("a", "b", "c")))
filter_states <- FilterStates$new(
input_dataname = "choices_dataset",
output_dataname = "choices_output",
datalabel = "label"
)
filter_states$queue_initialize(list(ReactiveQueue$new()))
choices_filter <- ChoicesFilterState$new(x = c("a", "c"), varname = "choices")
filter_states$queue_push(queue_index = 1, x = choices_filter, element_id = "test")
eval(isolate(filter_states$get_call()))
testthat::expect_equal(choices_output, choices_dataset[c(1, 3), , drop = FALSE])
})

Do you think there is a point to it or should we leave it? SE is quite unique in the sense it contains two queues with filters, so setting it up is a little bit different.

@denisovan31415
Copy link
Contributor Author

@kpagacz could you check now to see if the tests are adequate?

@gogonzo gogonzo self-assigned this Sep 9, 2021
Copy link
Contributor

@kpagacz kpagacz left a comment

Choose a reason for hiding this comment

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

Needs any news?

Copy link
Contributor

@gogonzo gogonzo left a comment

Choose a reason for hiding this comment

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

👍 LGTM
No News update is needed - it's not a change from master vs main perspective. It's a part of MAE refactor which has not been released yet

@denisovan31415 denisovan31415 merged commit 549fea9 into main Sep 10, 2021
@denisovan31415 denisovan31415 deleted the 163_more_tests_for_SEFilteredStates branch September 10, 2021 09:38
@gogonzo gogonzo self-assigned this May 2, 2022
walkowif pushed a commit to walkowif/teal that referenced this pull request Dec 5, 2023
)

Related to [teal.data PR
insightsengineering#184](insightsengineering/teal.data#184)
Make changes to `teal` because of the refactor to the JoinKeys class
from R6 to S3.

---------

Signed-off-by: Vedha Viyash <49812166+vedhav@users.noreply.github.com>
Co-authored-by: André Veríssimo <211358+averissimo@users.noreply.github.com>
Co-authored-by: Aleksander Chlebowski <114988527+chlebowa@users.noreply.github.com>
Co-authored-by: go_gonzo <dawid.kaledkowski@gmail.com>
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.

Add tests for SEFilterStates
3 participants