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.

Sign up for GitHub

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

Jump to bottom

ut: add test for Snapshot #39

Merged
merged 5 commits into from
Aug 6, 2024
Merged

ut: add test for Snapshot #39

merged 5 commits into from
Aug 6, 2024

Conversation

QuakeWang
Copy link
Contributor

part of #26

@QuakeWang
Copy link
Contributor Author

GitHub action doesn't seem to run automatically 🥲. @Xuanwo

@Xuanwo
Copy link
Member

Xuanwo commented Aug 4, 2024

GitHub action doesn't seem to run automatically 🥲. @Xuanwo

Thanks for the feedback. I will check with the INFRA team.

@Xuanwo
Copy link
Member

Xuanwo commented Aug 4, 2024

Hi, it should be work now. Please try push a new commit or a new PR, thanks!

@QuakeWang
Copy link
Contributor Author

Hi, it should be work now. Please try push a new commit or a new PR, thanks!

It works!

XuQianJin-Stars
XuQianJin-Stars reviewed Aug 5, 2024
View reviewed changes
crates/paimon/src/spec/snapshot.rs Outdated
assert_eq!(snapshot.total_record_count(), Some(500));
assert_eq!(snapshot.delta_record_count(), Some(200));
assert_eq!(snapshot.changelog_record_count(), Some(50));
assert_eq!(snapshot.watermark(), Some(123456789));
Copy link
Contributor

Choose a reason for hiding this comment

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

Add an option for ut with a value of None?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add an option for ut with a value of None?

OK, I will do it later.

Xuanwo
Xuanwo approved these changes Aug 5, 2024
View reviewed changes
Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Thank you for this PR. However, I would prefer to see a test that can serialize/deserialize actual snapshot files. The current PR only tests simple get/set operations, which are already covered by TypedBuilder.

@QuakeWang
Copy link
Contributor Author

Thank you for this PR. However, I would prefer to see a test that can serialize/deserialize actual snapshot files. The current PR only tests simple get/set operations, which are already covered by TypedBuilder.

Hi, I have add serde_json for the serialize/deserialize test, PTAL 👀

@QuakeWang QuakeWang mentioned this pull request Aug 6, 2024
Closed
XuQianJin-Stars
XuQianJin-Stars approved these changes Aug 6, 2024
View reviewed changes
Copy link
Contributor

@XuQianJin-Stars XuQianJin-Stars left a comment

Choose a reason for hiding this comment

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

LGTM

Xuanwo
Xuanwo approved these changes Aug 6, 2024
View reviewed changes
Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Thanks!

@SteNicholas SteNicholas merged commit bd2f887 into apache:main Aug 6, 2024
7 checks passed
@QuakeWang QuakeWang mentioned this pull request Aug 6, 2024
Open
@QuakeWang QuakeWang deleted the snapshot-ut branch August 6, 2024 06:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@XuQianJin-Stars XuQianJin-Stars XuQianJin-Stars approved these changes

@SteNicholas SteNicholas SteNicholas approved these changes

@Xuanwo Xuanwo Xuanwo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@QuakeWang @Xuanwo @SteNicholas @XuQianJin-Stars