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

chore(core/rawdb): ExampleInspectDatabase test #119

Merged

Conversation

qdm12
Copy link
Collaborator

@qdm12 qdm12 commented Feb 5, 2025

Why this should be merged

Add an example test for InspectDatabase with options.

How this works

See example code

@qdm12 qdm12 force-pushed the qdm12/core/rawdb/inspect-database-test branch 2 times, most recently from 52e6180 to 02fbb8b Compare February 6, 2025 12:47
@qdm12 qdm12 marked this pull request as ready for review February 6, 2025 13:01
core/rawdb/ancient_utils.go Outdated Show resolved Hide resolved
core/rawdb/ancient_utils.go Outdated Show resolved Hide resolved
core/rawdb/database.go Outdated Show resolved Hide resolved
core/rawdb/database_test.go Outdated Show resolved Hide resolved
core/rawdb/database_test.go Outdated Show resolved Hide resolved
@qdm12 qdm12 force-pushed the qdm12/core/rawdb/inspect-database-test branch from 02fbb8b to 2fabe02 Compare February 7, 2025 12:59
@qdm12 qdm12 force-pushed the qdm12/core/rawdb/inspect-database-test branch from 2fabe02 to eff72a3 Compare February 7, 2025 13:07
@qdm12
Copy link
Collaborator Author

qdm12 commented Feb 7, 2025

I was initially planning to (ill-fated plan):

  1. PR the previous code (long example) upstream and to main
  2. Merge changes from main to feat: rawdb.InspectDatabaseOption #111 and update example to test options

The 🆕 plan:

  1. PR-ing this to feat: rawdb.InspectDatabaseOption #111 directly with option testing and minimal geth code testing (i.e. not all switch cases)
  2. Branched out in another branch with the previous code to PR upstream. I also changed the example to be a unit test, not an example, so it won't conflict with our example and because it was too long for an example anyway, and changed InspectDatabase to take in an io.Writer argument to test the output.

I'll comment on previous comments just for the sake of discussion and a pre-review on an upstream PR, but these conversations are unimportant so feel free to ignore too 😉

qdm12

This comment was marked as outdated.

@qdm12 qdm12 changed the base branch from main to arr4n/rawdb-inspect-stats February 7, 2025 13:18
Copy link
Collaborator

@ARR4N ARR4N left a comment

Choose a reason for hiding this comment

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

This is much easier to understand, making it "obviously" correct, which is what tests should strive to be IMO. Thanks for taking the extra time.

@ARR4N ARR4N merged commit 600f97e into arr4n/rawdb-inspect-stats Feb 7, 2025
4 checks passed
@ARR4N ARR4N deleted the qdm12/core/rawdb/inspect-database-test branch February 7, 2025 16: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