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

[dagster-pipes-rust] - Inital integration with the pipes test suite #66

Merged
merged 9 commits into from
Dec 23, 2024

Conversation

marijncv
Copy link
Collaborator

@marijncv marijncv commented Dec 20, 2024

Summary & Motivation

This sets up the initial integration with the dagster-pipes-tests suite. Most of the tests are skipped right now, they can be enabled incrementally.

The tests are run from a binary that is only included in the build if the optional pipes-tests feature is enabled. This keeps the main library slim.

Contributes to #58

How I Tested These Changes

cd libraries/pipes/implementations/rust
uv run pytest

Changelog

Ensure that an entry has been created in CHANGELOG.md outlining additions, deletions, and/or modifications.

See: keepachangelog.com

@cmpadden cmpadden requested a review from danielgafni December 20, 2024 14:45
Copy link
Contributor

@danielgafni danielgafni left a comment

Choose a reason for hiding this comment

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

I wonder if it would be more appropriate to disable all tests in pipes.toml instead (so that they are skipped) rather than having no-op tests.

  1. It would more explicitly communicate that there are no tests written yet.
  2. It would make it easier to incrementally enable them one by one.

On a side note, the tests CLI args are not stabilized yet, we will be standardizing them next week with @GingerYouth for Java

@marijncv
Copy link
Collaborator Author

marijncv commented Dec 20, 2024

I wonder if it would be more appropriate to disable all tests in pipes.toml

That's a good point! I set all (skippable) tests to be skipped now.

the tests CLI args are not stabilized yet

That is good to know. I'll probably keep this PR in draft for a bit longer to work out how to deal with the clap dependency and to see if this setup works with one of the --full tests

@danielgafni
Copy link
Contributor

I think it's ok to start working on the tests. We will probably remove the --full flag (as it's almost always set to true) and do a bit changes here and there. It's not going to be a lot of changes. Just fyi.

@marijncv marijncv marked this pull request as ready for review December 21, 2024 17:02
@marijncv marijncv changed the title Rust pipes test suite [dagster-pipes-rust] - Inital integration with the pipes test suite Dec 21, 2024
Copy link
Contributor

@danielgafni danielgafni left a comment

Choose a reason for hiding this comment

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

LGTM! just a small naming comment

.github/workflows/libraries-pipes-rust.yml Outdated Show resolved Hide resolved
@marijncv marijncv merged commit 2b8bb35 into main Dec 23, 2024
3 checks passed
@marijncv marijncv deleted the rust-pipes-test-suite branch December 23, 2024 09:06
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