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

feat: add basic behavior tests for buffer reader #3862

Merged
merged 1 commit into from
Dec 30, 2023

Conversation

WenyXu
Copy link
Member

@WenyXu WenyXu commented Dec 30, 2023

Add basic behavior tests for buffer reader (p.s. I will rebase this PR after #3865 is merged)

Related

#3735

@Xuanwo
Copy link
Member

Xuanwo commented Dec 30, 2023

Hi, the TwoWay/ThreeWay/FourWay refactor isn't complete. Would you prefer to merge them first?

@WenyXu
Copy link
Member Author

WenyXu commented Dec 30, 2023

Hi, the TwoWay/ThreeWay/FourWay refactor isn't complete. Would you prefer to merge them first?

Sorry, I didn't get it. It sounds like I missed something.

@Xuanwo
Copy link
Member

Xuanwo commented Dec 30, 2023

Sorry, I didn't get it. It sounds like I missed something.

We can merge TwoWaysReader and TwoWaysWrite into TwoWay, and refactor all references.

@WenyXu
Copy link
Member Author

WenyXu commented Dec 30, 2023

Sorry, I didn't get it. It sounds like I missed something.

We can merge TwoWaysReader and TwoWaysWrite into TwoWay, and refactor all references.

OK, I see. I will add tests first and refactor TwoWaysReader and TwoWaysWrite while waiting for CI. 😄

@WenyXu
Copy link
Member Author

WenyXu commented Dec 30, 2023

Sorry, I didn't get it. It sounds like I missed something.

We can merge TwoWaysReader and TwoWaysWrite into TwoWay, and refactor all references.

BTW where should I place the TwoWay? How about the raw/oio/utils.rs?

@Xuanwo
Copy link
Member

Xuanwo commented Dec 30, 2023

BTW where should I place the TwoWay? How about the raw/oio/utils.rs?

How about in raw/enum_utils?

@WenyXu
Copy link
Member Author

WenyXu commented Dec 30, 2023

BTW where should I place the TwoWay? How about the raw/oio/utils.rs?

How about in raw/enum_utils?

OK 👍

@WenyXu WenyXu force-pushed the feat/behavior-read-only branch 2 times, most recently from 3a97cd4 to da6f9d5 Compare December 30, 2023 14:52
@WenyXu
Copy link
Member Author

WenyXu commented Dec 30, 2023

Cool, It seems easier than I thought.

BTW, Should I refactor the tests to like the following?

pub async fn test_reader_tail_with_buffer(op: Operator) -> Result<()> {
    build_test_reader_tail(test, |op| op.buffer().. ))
}

@WenyXu
Copy link
Member Author

WenyXu commented Dec 30, 2023

Hhh, 1 check failed...🥲

@WenyXu WenyXu force-pushed the feat/behavior-read-only branch 2 times, most recently from 18dd3be to 8ed6557 Compare December 30, 2023 15:44
@WenyXu WenyXu marked this pull request as ready for review December 30, 2023 15:44
@WenyXu
Copy link
Member Author

WenyXu commented Dec 30, 2023

The fuzz tests were amazing and helpful! I will rebase this PR after #3865 is merged.

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.

Hi, I feel like we are going too far.

Sorry for not making the difference between behavior tests and fuzz tests more clearly in advance. (I promise that I will improve this part in the future).

Behavior tests are tests to simulate user's behavior, they're optimized for reading and maintaining. It's by design to have duplicated code. We try our best to make every test case is separate to each other and could be understood without reading other part of code.

And fuzzing test is to reproduce failures produced by fuzzer. They are designed for easily adding and reproducing auto generated cases.

So please keep behavior tests simple like users can write in their real code.

@WenyXu
Copy link
Member Author

WenyXu commented Dec 30, 2023

Hi, I feel like we are going too far.

Sorry for not making the difference between behavior tests and fuzz tests more clearly in advance. (I promise that I will improve this part in the future).

Behavior tests are tests to simulate user's behavior, they're optimized for reading and maintaining. It's by design to have duplicated code. We try our best to make every test case is separate to each other and could be understood without reading other part of code.

And fuzzing test is to reproduce failures produced by fuzzer. They are designed for easily adding and reproducing auto generated cases.

So please keep behavior tests simple like users can write in their real code.

Ok, I see. I dropped the refactor commit.

@Xuanwo
Copy link
Member

Xuanwo commented Dec 30, 2023

Thanks a lot for the understanding!

@WenyXu
Copy link
Member Author

WenyXu commented Dec 30, 2023

Thanks a lot for the understanding!

Never mind. 😁

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!

@Xuanwo Xuanwo merged commit 04d0a1f into apache:main Dec 30, 2023
177 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants