Skip to content

Conversation

@asnare
Copy link
Contributor

@asnare asnare commented Sep 20, 2024

Changes

This PR implements some fixture improvements that were originally implemented downstream:

  • When creating an account or workspace group, we now require the group to be visible via two subsequent .get() and .list() calls. This double-check approach mitigates (but doesn't completely eliminate) problems with consistency of the APIs for working with groups.
  • The wait_for_provisioning argument to the make_group and make_acc_group has now been removed: we always wait. (For compatibility the argument is still accepted but will trigger a deprecation warning.)

An incidental change is the internal unit-test plumbing can now be provided with mock fixtures specific to the test; this is needed to ensure the double-check implementation can be tested from the unit tests.

Linked issues

Supersedes databrickslabs/ucx#2634.

Tests

  • added/updated unit tests
  • existing integration tests

@asnare asnare requested a review from nfx as a code owner September 20, 2024 16:21
@github-actions
Copy link

github-actions bot commented Sep 20, 2024

This PR breaks backwards compatibility for databrickslabs/blueprint downstream. See build logs for more details.

Running from downstreams #46

@github-actions
Copy link

github-actions bot commented Sep 20, 2024

✅ 35/35 passed, 3 skipped, 2m30s total

Running from acceptance #87

@asnare asnare self-assigned this Sep 20, 2024
@asnare asnare added the enhancement New feature or request label Sep 20, 2024
@asnare
Copy link
Contributor Author

asnare commented Sep 20, 2024

This PR breaks backwards compatibility for databrickslabs/blueprint downstream. See build logs for more details.

Running from downstreams #32

I don't fully understand why this check is failing…?

Copy link
Contributor

@JCZuurmond JCZuurmond left a comment

Choose a reason for hiding this comment

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

Add some nits, otherwise looks good

Copy link
Contributor

@JCZuurmond JCZuurmond left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@nfx nfx left a comment

Choose a reason for hiding this comment

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

don't modify a global variable - this will break unit tests at multi-threaded execution.

try:
yield
finally:
_FIXTURES.reset(token)
Copy link
Collaborator

Choose a reason for hiding this comment

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

this modifies shared global state and will definitely break when running in multiple threads. why is this necessary? revert.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need this because the default auto-mocks don't allow the double-check waiting inside make_group() to complete: the return values of .create() and .list() on the (account-)groups REST APIs need to be controlled.

At the moment I don't see a test-specific way to pre-configure the mocks that call_stateful() will set up as fixtures for the fixture being tested. (I didn't find any other unit tests that need this, but may have missed it or there's another path to the same thing.)

Given that pre-configuring mocks for something under test is a fairly common thing to do, I came up with this. It's specifically intended to not only be thread-safe but also async-safe: I don't understand why it will break when running in multiple threads?

The main alternatives that spring to mind are:

  • Use a specific keyword argument to call_stateful() for this purpose that isn't passed through to the fixture under test.
  • Modify the global fixture (inside CallContext) that all users of call_stateful() have available so that it includes the pre-configured mocks that these group-creation tests use. This would be in the same way as make_random (for example) is handled.

Do you have a preference, or is there something else I've overlooked?

Copy link
Collaborator

Choose a reason for hiding this comment

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

do it without changing a global variable - modify call_context right above the result = ctx[some.__name__](**kwargs) (the first alternative)

def apply(ctx: CallContext):
    mock_group = Group(id="an_id")
    ctx['ws].groups.create.return_value = mock_group
    ctx['ws].groups.list.return_value = [mock_group]

ctx, group = call_stateful(make_group, call_context_callback=apply)

try:
yield
finally:
_FIXTURES.reset(token)
Copy link
Collaborator

Choose a reason for hiding this comment

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

do it without changing a global variable - modify call_context right above the result = ctx[some.__name__](**kwargs) (the first alternative)

def apply(ctx: CallContext):
    mock_group = Group(id="an_id")
    ctx['ws].groups.create.return_value = mock_group
    ctx['ws].groups.list.return_value = [mock_group]

ctx, group = call_stateful(make_group, call_context_callback=apply)

Copy link
Collaborator

@nfx nfx left a comment

Choose a reason for hiding this comment

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

lgtm

@nfx nfx merged commit c11bc6d into main Sep 24, 2024
8 of 9 checks passed
@nfx nfx deleted the make-group-reliability branch September 24, 2024 10:12
nfx added a commit that referenced this pull request Sep 24, 2024
* Fixed PyPI metadata ([#54](#54)). In this commit, the PyPI metadata for the pytester project has been updated with the new repository location at <https://github.com/databrickslabs/pytester>. The URLs for issues and source have been changed to point to the new repository, with the `issues` URL now directing to <https://github.com/databrickslabs/pytester/issues> and the `source` URL to <https://github.com/databrickslabs/pytester>. Furthermore, the versioning tool `hatch` has been configured to manage the version number in the "src/databricks/labs/pytester/__about__.py" file. This ensures accurate and consistent versioning for the pytester project moving forward.
* Improve `make_group`/`make_acc_group` fixture consistency ([#50](#50)). This PR introduces improvements to the `make_group` and `make_acc_group` fixtures, designed for managing Databricks workspace groups. The enhancements include a double-check approach to ensure group visibility by requiring the group to be retrievable via both `.get()` and `.list()` calls. This mitigates, but does not entirely eliminate, consistency issues with the APIs used for managing groups. The `wait_for_provisioning` argument has been removed and replaced with an internal wait mechanism. The argument is still accepted but triggers a deprecation warning. Internal unit-test plumbing has been updated to use mock fixtures tailored for each test, ensuring double-check implementation testability. New and updated unit tests are included in the `test_iam.py` file, along with the introduction of the `_setup_groups_api` function, which mocks specific clients to ensure group visibility when created. These changes improve consistency and reliability when working with Databricks workspace groups, making it easier for users to adopt the project.
@nfx nfx mentioned this pull request Sep 24, 2024
nfx added a commit that referenced this pull request Sep 24, 2024
* Fixed PyPI metadata
([#54](#54)). In this
commit, the PyPI metadata for the pytester project has been updated with
the new repository location at
<https://github.com/databrickslabs/pytester>. The URLs for issues and
source have been changed to point to the new repository, with the
`issues` URL now directing to
<https://github.com/databrickslabs/pytester/issues> and the `source` URL
to <https://github.com/databrickslabs/pytester>. Furthermore, the
versioning tool `hatch` has been configured to manage the version number
in the "src/databricks/labs/pytester/__about__.py" file. This ensures
accurate and consistent versioning for the pytester project moving
forward.
* Improve `make_group`/`make_acc_group` fixture consistency
([#50](#50)). This PR
introduces improvements to the `make_group` and `make_acc_group`
fixtures, designed for managing Databricks workspace groups. The
enhancements include a double-check approach to ensure group visibility
by requiring the group to be retrievable via both `.get()` and `.list()`
calls. This mitigates, but does not entirely eliminate, consistency
issues with the APIs used for managing groups. The
`wait_for_provisioning` argument has been removed and replaced with an
internal wait mechanism. The argument is still accepted but triggers a
deprecation warning. Internal unit-test plumbing has been updated to use
mock fixtures tailored for each test, ensuring double-check
implementation testability. New and updated unit tests are included in
the `test_iam.py` file, along with the introduction of the
`_setup_groups_api` function, which mocks specific clients to ensure
group visibility when created. These changes improve consistency and
reliability when working with Databricks workspace groups, making it
easier for users to adopt the project.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants