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

test: added rust sdk test for source transformer #1756

Merged
merged 5 commits into from
Jun 17, 2024

Conversation

shubhamdixit863
Copy link
Contributor

@shubhamdixit863 shubhamdixit863 commented Jun 9, 2024

What this PR does

This PR introduces a new test for the Rust SDK aimed at ensuring the functionality of the source transformer module. By integrating this test, we aim to maintain high code quality and reliability of the transformations performed by this module.
Fixes -numaproj/numaflow-rs#56

Changes Introduced

  • Added Rust SDK test for source transformer to verify its correctness and stability.

The added tests were executed locally to confirm their effectiveness. Results confirm that the new Rust SDK test for the source transformer runs successfully without introducing regressions.

@shubhamdixit863 shubhamdixit863 changed the title added rust sdk test for source transformer ,Fixes -https://github.com/numaproj/numaflow-rs/issues/56 added rust sdk test for source transformer Jun 9, 2024
Copy link
Member

@vigith vigith left a comment

Choose a reason for hiding this comment

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

I don't think this testdata is being tested, though.

@vigith
Copy link
Member

vigith commented Jun 9, 2024

https://github.com/numaproj/numaflow/blob/main/test/sdks-e2e/sdks_test.go

@whynowy whynowy changed the title added rust sdk test for source transformer test: added rust sdk test for source transformer Jun 10, 2024
whynowy
whynowy previously approved these changes Jun 10, 2024
Copy link
Member

@whynowy whynowy left a comment

Choose a reason for hiding this comment

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

Anything remaining?

@whynowy whynowy dismissed their stale review June 10, 2024 22:36

testing failed.

Copy link
Member

@KeranYang KeranYang left a comment

Choose a reason for hiding this comment

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

E2E should pass once we rebuild & push the image with the protobuf time conversion fix, right? @shubhamdixit863

transformer:
container:
# Filter messages based on event time, see https://github.com/numaproj/numaflow-go/tree/main/pkg/sourcetransformer/examples/event_time_filter
image: quay.io/numaio/numaflow-rust/source-transformer:stable
Copy link
Member

Choose a reason for hiding this comment

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

Naming - we can have multiple source transformer images in the future.

Suggested change
image: quay.io/numaio/numaflow-rust/source-transformer:stable
image: quay.io/numaio/numaflow-rust/mapt-event-time-filter:stable

@vigith
Copy link
Member

vigith commented Jun 14, 2024

@KeranYang we need the same publish scripts for Rust SDK too.

http: {}
transformer:
container:
# Filter messages based on event time, see https://github.com/numaproj/numaflow-go/tree/main/pkg/sourcetransformer/examples/event_time_filter
Copy link
Member

Choose a reason for hiding this comment

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

wrong link. Please kindly proofread your PRs before marking ready for review. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah sure ,thankyou :)

Copy link

codecov bot commented Jun 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 56.89%. Comparing base (adbc5ca) to head (8edfee7).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1756      +/-   ##
==========================================
+ Coverage   56.86%   56.89%   +0.03%     
==========================================
  Files         216      216              
  Lines       17350    17350              
==========================================
+ Hits         9866     9872       +6     
+ Misses       6659     6643      -16     
- Partials      825      835      +10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@shubhamdixit863
Copy link
Contributor Author

E2E should pass once we rebuild & push the image with the protobuf time conversion fix, right? @shubhamdixit863

Yes its fixed

@KeranYang KeranYang merged commit d72bd71 into numaproj:main Jun 17, 2024
25 checks passed
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.

4 participants