-
Notifications
You must be signed in to change notification settings - Fork 25
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
Allow exact group matches #261
base: main
Are you sure you want to change the base?
Conversation
con <- Connect$new(server = "https://connect.example", api_key = "fake") | ||
|
||
test_that("groups_create_remote errors if prefix returns zero matches", { | ||
skip("not implemented") |
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.
Why not?
test_that("groups_create_remote creates multiples if multiple are found", { | ||
expect_error( | ||
groups_create_remote(con, "Art"), | ||
"The expected group\\(s\\) were not found" |
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 don't understand how this matches the test description
Hi, |
Co-authored-by: Neal Richardson <[email protected]>
# Conflicts: # tests/testthat/2024.08.0/__api__/v1/groups-2c8d16-PUT.json # tests/testthat/2024.08.0/__api__/v1/groups-8a8b3e-PUT.json # tests/testthat/2024.08.0/__api__/v1/groups-8e9a47.json # tests/testthat/2024.08.0/__api__/v1/groups-c01bad.json # tests/testthat/2024.08.0/__api__/v1/groups/remote-519747.json # tests/testthat/2024.08.0/__api__/v1/groups/remote-d27795.json
tests/testthat/test-remote.R
Outdated
test_that("groups_create_remote works if match already exists", { | ||
expect_message( | ||
groups_create_remote(con, "Everyone"), | ||
"Creating remote group" | ||
) | ||
}) |
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.
Is this supposed to test the check == FALSE
behavior?
test_that("groups_create_remote creates match if one is found", { | ||
skip("not implemented") | ||
}) | ||
test_that("groups_create_remote creates multiples if multiple are found", { |
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 test wording seems to suggest that it's intended to test the behavior when expect > 1
. But there's also the skeleton of a test for an error if expect > 1
.
message(glue::glue("At least one group with name prefix '{prefix}' already exists")) | ||
} else { | ||
message(glue::glue("A group with the name '{prefix}' already exists")) | ||
|
||
} | ||
return(local_groups) |
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.
This behavior seems weird to me — I would expect the code to throw an error if the requested operation failed, rather than emitting a message and returning an object as though it had succeeded. Am I thinking about this wrong?
Adds an
exact
parameter togroups_create_remote
to allow creating a specific group even if there are multiple matches. This follows a similar change in #157This as well as the matching
users_create_remote
could use some refactoring, though I'm not sure it's worth it in this PR.Resolves #216