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

monorepo: convert test data from JSON to objects #3642

Merged
merged 28 commits into from
Sep 10, 2024

Conversation

gabrocheleau
Copy link
Contributor

@gabrocheleau gabrocheleau commented Sep 7, 2024

This PR begins addressing #3637 by converting JSON test data to objects.

The packages fully converted by this PR are:

  • Block
  • Blockchain
  • Devp2p

Some other packages have been partially migrated because of cross-dependencies.

Copy link

codecov bot commented Sep 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.90%. Comparing base (4470cc3) to head (85a3910).
Report is 40 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

Flag Coverage Δ
block ?
client ?
tx 76.90% <ø> (-0.88%) ⬇️
wallet ?

Flags with carried forward coverage won't be shown. Click here to find out more.

@gabrocheleau gabrocheleau marked this pull request as ready for review September 9, 2024 02:06
block_number: string
gas_limit: string
gas_used: string
timestamp: string
Copy link
Member

Choose a reason for hiding this comment

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

What is going on here ?!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Through this work I realized that these values are actually numeric string, not 0xprefixed strings. See for e.g.

https://github.com/ethereumjs/ethereumjs-monorepo/blob/monorepo/typescript-test-files/packages/block/test/testdata/payload-slot-87335.ts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created a numeric string type for it

Copy link
Member

Choose a reason for hiding this comment

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

Oh, that's super cool (so: the new type)! Again on this level, small change, but really another typing milestone in our libraries, once we have applied this consistently throughout the libraries! 🤩

@holgerd77
Copy link
Member

🙏

Copy link
Contributor

@acolytec3 acolytec3 left a comment

Choose a reason for hiding this comment

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

LGTM

@acolytec3 acolytec3 merged commit 4a5f98d into master Sep 10, 2024
38 checks passed
@scorbajio scorbajio deleted the monorepo/typescript-test-files branch September 10, 2024 17:44
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.

3 participants