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

initial set of edge cases #110

Merged
merged 16 commits into from
Sep 20, 2024
Merged

initial set of edge cases #110

merged 16 commits into from
Sep 20, 2024

Conversation

anupsv
Copy link
Contributor

@anupsv anupsv commented Sep 3, 2024

Fixes Issue

Fixes #

Changes proposed

Screenshots (Optional)

Note to reviewers

@anupsv anupsv requested a review from bxue-l2 September 16, 2024 22:26
@anupsv anupsv marked this pull request as ready for review September 16, 2024 22:26
e2e/server_test.go Outdated Show resolved Hide resolved
e2e/server_test.go Show resolved Hide resolved
e2e/server_test.go Outdated Show resolved Hide resolved
e2e/server_test.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@samlaf samlaf left a comment

Choose a reason for hiding this comment

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

Looks like useful stuff! Would be nice to have a big comment explaining the different edge cases, otherwise it's very hard to tell whether they are extensive or not.

Also should we turn this into a fuzz test using your edge cases as seed examples?

anupsv and others added 2 commits September 19, 2024 11:01
Co-authored-by: Samuel Laferriere <samlaf92@gmail.com>
@anupsv
Copy link
Contributor Author

anupsv commented Sep 19, 2024

Looks like useful stuff! Would be nice to have a big comment explaining the different edge cases, otherwise it's very hard to tell whether they are extensive or not.

Also should we turn this into a fuzz test using your edge cases as seed examples?

I've been looking to create a full end to end fuzzer, i.e integrate proxy with inabox and run the fuzzer through the proxy so the full flow is being tested continuously. Wanted to get these tests in first so that known issues are fixed. Also fuzzing, depending on seed size, would create long PR cycles but i guess people are ok with it since the holesky test takes around 40 minutes. I'll add it in a different PR later on and we can discuss there for runtimes and sizes.

@samlaf
Copy link
Collaborator

samlaf commented Sep 20, 2024

Looks like useful stuff! Would be nice to have a big comment explaining the different edge cases, otherwise it's very hard to tell whether they are extensive or not.
Also should we turn this into a fuzz test using your edge cases as seed examples?

I've been looking to create a full end to end fuzzer, i.e integrate proxy with inabox and run the fuzzer through the proxy so the full flow is being tested continuously. Wanted to get these tests in first so that known issues are fixed. Also fuzzing, depending on seed size, would create long PR cycles but i guess people are ok with it since the holesky test takes around 40 minutes. I'll add it in a different PR later on and we can discuss there for runtimes and sizes.

Agree to merging this first and then later adding fuzzing using these as base case. golang fuzzer is integrated and pretty fast! You can run it for a fixed amount of time (say 10sec), or run it until its hit n amounts of "surprising" results (say 200). Don't think it should take that long for something simple like this, but I might be wrong...

Giving my stamp of approval, in case this is ready to merge. Prefer smaller PRs that we merge more often, otherwise we have stuff lingering around for weeks on end.

@anupsv anupsv merged commit ab0aeca into main Sep 20, 2024
7 checks passed
@anupsv anupsv deleted the edge-case-tests branch September 20, 2024 18:33
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.

3 participants