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

Added singular tests to groups API. #1372

Merged
merged 3 commits into from
Mar 3, 2024

Conversation

elongl
Copy link
Member

@elongl elongl commented Jan 15, 2024

No description provided.

Copy link

linear bot commented Jan 15, 2024

Copy link
Contributor

👋 @elongl
Thank you for raising your pull request.
Please make sure to add tests and document all user-facing changes.
You can do this by editing the docs files in this pull request.

from elementary.utils.schema import ExtendedBaseModel
from elementary.utils.time import convert_partial_iso_format_to_full_iso_format


class NormalizedTestSchema(NormalizedArtifactSchema):
Copy link
Member Author

Choose a reason for hiding this comment

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

I dislike how this is here while all the others (model, source, exposure) are at fetchers/models/schema.py even though they don't store models.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah not a fan of that either, if you wish to refactor I'm not against (but if you don't wish to refactor I'm also not against)

elementary/monitor/fetchers/tests/tests.py Show resolved Hide resolved
from elementary.utils.schema import ExtendedBaseModel
from elementary.utils.time import convert_partial_iso_format_to_full_iso_format


class NormalizedTestSchema(NormalizedArtifactSchema):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah not a fan of that either, if you wish to refactor I'm not against (but if you don't wish to refactor I'm also not against)

@@ -21,7 +21,7 @@ class NormalizedArtifactSchema(ExtendedBaseModel):
# Currently, it's model_name to match the CLI UI.
model_name: str
normalized_full_path: str
fqn: str
fqn: Optional[str] = None
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure if the FE actually relies on this field existing, worth double-checking

Copy link
Member Author

Choose a reason for hiding this comment

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

It's used in catalog, and lineage's bottom drawer.
Tests don't appear in either of those so that's fine.

@elongl elongl force-pushed the ele-2361-show-singular-tests-in-report branch from ff44ed6 to 166ab16 Compare January 16, 2024 12:10
@elongl elongl force-pushed the ele-2361-show-singular-tests-in-report branch from 8a5b698 to 5c7d93c Compare March 3, 2024 10:30
@elongl elongl merged commit f29b2aa into master Mar 3, 2024
3 checks passed
@elongl elongl deleted the ele-2361-show-singular-tests-in-report branch March 3, 2024 11:03
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.

None yet

3 participants