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
[BUGFIX] Ensure that Checkpoint
deserializes proper action subclass
#9701
Conversation
✅ Deploy Preview for niobium-lead-7998 canceled.
|
ids=[k.__name__ for k in ACTION_INIT_PARAMS], | ||
) | ||
@pytest.mark.unit | ||
def test_action_deserialization_within_parent_class( |
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.
Now that we can test this with Checkpoint, I'd like to remove this test
def test_checkpoint_deserialization_with_actions(self, mocker: MockerFixture): | ||
# Arrange | ||
context = mocker.Mock(spec=AbstractDataContext) | ||
context.validation_definition_store.get.return_value = mocker.Mock( |
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.
deserializing a checkpoint hits the validation definition store?
assert len(checkpoint.actions) == 3 | ||
assert isinstance(checkpoint.actions[0], UpdateDataDocsAction) | ||
assert isinstance(checkpoint.actions[1], SlackNotificationAction) | ||
assert isinstance(checkpoint.actions[2], MicrosoftTeamsNotificationAction) |
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'd probably add another test here to verify that (pseudocode) serialized_checkpoint == serialize(deserialize(checkpoint))
from great_expectations.checkpoint.actions import ( | ||
EmailAction, # noqa: TCH001 | ||
MicrosoftTeamsNotificationAction, # noqa: TCH001 | ||
OpsgenieAlertAction, # noqa: TCH001 | ||
PagerdutyAlertAction, # noqa: TCH001 | ||
SlackNotificationAction, # noqa: TCH001 | ||
SNSNotificationAction, # noqa: TCH001 | ||
StoreValidationResultAction, # noqa: TCH001 | ||
UpdateDataDocsAction, # noqa: TCH001 | ||
) |
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.
Rather than ignoring all these warnings I would add these (or their immediate parent) here.
great_expectations/pyproject.toml
Lines 381 to 387 in 7993a68
runtime-evaluated-base-classes = [ | |
# NOTE: ruff is unable to detect that these are subclasses of pydantic.BaseModel | |
"pydantic.BaseModel", | |
"great_expectations.datasource.fluent.fluent_base_model.FluentBaseModel", | |
"great_expectations.datasource.fluent.interfaces.Datasource", | |
"great_expectations.datasource.fluent.sql_datasource.SQLDatasource", | |
] |
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.
Actually sorry, you need Checkpoint
here, or pydantic.v1.BaseModel
not these actions.
"pydantic.BaseModel",
"pydantic.v1.BaseModel",
"great_expectations.compatibility.pydantic.BaseModel",
# if adding these doesn't fix it you can add the Checkpoint model directly.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #9701 +/- ##
========================================
Coverage 82.55% 82.56%
========================================
Files 511 511
Lines 46454 46450 -4
========================================
+ Hits 38351 38352 +1
+ Misses 8103 8098 -5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Without the specific union noted in this PR, we get a generic
ValidationAction
type when reading from disk.invoke lint
(usesruff format
+ruff check
)For more information about contributing, see Contribute.
After you submit your PR, keep the page open and monitor the statuses of the various checks made by our continuous integration process at the bottom of the page. Please fix any issues that come up and reach out on Slack if you need help. Thanks for contributing!