-
Notifications
You must be signed in to change notification settings - Fork 809
testdata: add testdata package #3940
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
Conversation
I had not thought about actually making this a package, but I think this makes sense. So this would be the "devDependency" package where we can put test files + helpers in? The alternative to just make this a folder and on importing test data relative to the test (so `import { hardfork4844Data } from '../../testdata/src' for instance) would be a bit ugly 🤔 (Thinking out loud a bit, I was thinking of the testdata as "just a folder" and not package) |
Yeah I'm not actually sure. It is actually typescript code it felt out of place to put it at the root level under |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
49f0c67
to
27e879c
Compare
Not doing a full review, just some notes, generally looks really great, also think at second look that the I think it's important that we establish a somewhat solid test data folder structure from the beginning, so that things remain somewhat sorted and re-discoverable, this also helps to accidentally re-add the same test data. From having a quick look I would suggest the following first-round folders:
Then people are also encouraged to directly add to this structure, e.g. if there is tx testdata or something else which is structurally different. |
Thanks for commenting! Concerning the package naming, this approach actually did not work, notably because symlinking/alias is just equivalent to a relative imports, and we can't do relative imports in our packages. See some complaints when TS when I attempted that approach: More conversation happened about this in the tech-internal channel. Sure, I like the recommendation about folders. Wdyt if we do without the nested folders for chains to keep a flatter structure? E.g.:
|
…eumjs/ethereumjs-monorepo into testdata/test-data-package
}, | ||
"files": ["dist", "src"], | ||
"scripts": { | ||
"build": "../../config/cli/ts-build.sh", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we actually need a build step here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My underestanding is that it's best to do it to enable tests to be run with nodejs (otherwise users would need to use tsx/ts-node)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I deleted the dist folder from testdata
and ran all the tests locally and they pass just fine. This is what I expected since our code is intended to use typescript sources and vitest uses a transpiler internally to handle typescript code. Since this package is for local testing only and not intended to be published, I'd advocate to skip the build step entirely so as to not add anything else to the CI (even though this admittedly won't add much)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah! Nice, thanks for actually trying this out. Will remove the build step and add the necessary --if-present flag so that the root postinstall doesn't fail on that package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done! Feel free to re-approve! :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is awesome. Great start on this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, agree, super great start! 🙂❤️
* chore: add testdata package * testdata: fix build * chore: use exact version number instead of workspace:* * chore: add 4844 testdata to testdata packgae * chore: refactor testdata * chore: npm i * chore: migrate more test files2 * refactor: eip4844 test data2 * refactor: migate withdrawals geth genesis * refactor: gethGenesis misc data cleanup * refactor: migrate goerli and other test data * refactor: post merge geth genesis * fix: failing tests * refactor: more geth genesis test data * refactor: prelondon test data * chore: restructure folder * refactor: consolidate 4844 files and create chainConfigs folder * chore: add missing allocations * client: fix test * monorepo: remove build step
No description provided.