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

[components] Move dagster-components test component lib to dagster_components.lib.test #26611

Merged
merged 1 commit into from
Dec 20, 2024

Conversation

smackesey
Copy link
Collaborator

@smackesey smackesey commented Dec 19, 2024

Summary & Motivation

Move the dagster-components test component library from dagster_components_tests.lib to dagster_components.lib.test. This lets us simplify entry point loading code (eliminate call to ensure_dagster_components_tests_import) and also ensures these components are accessible whether from a published dagster-components or local (we don't include our test packages in published distributions as a rule)

How I Tested These Changes

Existing test suite.

Copy link
Member

@schrockn schrockn left a comment

Choose a reason for hiding this comment

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

also ensures these components are accessible whether from a published dagster-components

Why is this desirable?

Base automatically changed from sean/components/rebuild-component-registry to master December 20, 2024 12:09
@smackesey smackesey force-pushed the sean/components/move-dagster-components-test-lib branch from 6ca4051 to ed129b8 Compare December 20, 2024 12:10
@smackesey
Copy link
Collaborator Author

Why is this desirable?

It is useful for testing. Currently there are some annoyances with some tests that construct a code location without an editable install, and I think this will prove useful in manual testing in the future.

But the primary reason for this is to simplify the entry point loading code path by removing the need to check for the separate package dagster_components_tests.

Copy link
Member

@schrockn schrockn left a comment

Choose a reason for hiding this comment

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

Per discussion not sure this is where we end up landing, but it is incrementally better.

Copy link
Member

@schrockn schrockn left a comment

Choose a reason for hiding this comment

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

Per discussion the longer term solution is to publish a standalone test package.

@smackesey smackesey merged commit 3645506 into master Dec 20, 2024
1 check passed
@smackesey smackesey deleted the sean/components/move-dagster-components-test-lib branch December 20, 2024 17:48
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.

2 participants