-
-
Notifications
You must be signed in to change notification settings - Fork 407
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
Lazily download test262
repository
#3214
Conversation
Good idea, I don't see there being an issue with this. It should make the whole git submodules issue easier |
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.
Love it!! Thank you for taking this on, big devex upgrade for anyone using boa as a dep and doesn't want their clone to take minutes ^^
9de0f72
to
34db281
Compare
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## main #3214 +/- ##
==========================================
- Coverage 50.31% 50.23% -0.09%
==========================================
Files 436 436
Lines 42604 42677 +73
==========================================
Hits 21438 21438
- Misses 21166 21239 +73
☔ View full report in Codecov by Sentry. |
f1afff0
to
08a6203
Compare
Test262 conformance changes
|
This is now complete, and ready for review/merge :) Added Now tester warns if local test262 clone is out of sync with
|
a3dc4e8
to
9d2247f
Compare
Yeah, thought about it, but decided calling manually would be fine, also I didn't want to add another dependency on test262 without heavily using git features. The git command should be available on pretty much any development environment. |
Hi folks - wonder if we (we = Reth dev team) can help with this PR in any way? PR seems good / harmless to me - but understand you want to keep the bar high / ensure it gets adequately reviewed. |
9d2247f
to
3d80551
Compare
3d80551
to
ac67bdb
Compare
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 looks great implementation-wise, but I'd argue we should have a way to pin the commit downloaded by the tool for reproducibility in our CI. Maybe a file that lives in the root of the repo?
Maybe merge |
Great idea! We just have to create a new |
Hey @gakonst what does Reth use Boa for? I’m curious |
Implemented in the latest commit 3a0d75c :) |
Hi Jason, Reth (w/o getting in too many of the details around the whole thing) exposes a debugging JSON-RPC service to provide "execution traces" of the request, showing step by step what happened. These traces can be configured to run with certain "tracers", some of which are part of the node. However, there's benefit in allowing users to specify custom tracers. The way this was implemented originally in go-ethereum was by embedding a Javascript sandbox and on various parts of the tracer lifecycle to call to some client-side provided JS that augments the tracer output. In Reth, we use boa to provide that JS sandbox to the tracer. See Go-ethereum's docs for more on this. Hope that's helpful, happy to provide other context if needed but do not want to derail this issue. |
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.
Nice work! I just have a small nitpick that doesn't block merging 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.
looks good to me!
This Pull Request fixes/closes #3095 .
This PR makes the downloads the
test262
repository, if we run theboa_tester
withrun
command and we do not specify the test262 directory.There is a con with this approach, dependabot can't suggest a updates. We could implement our own checker though.
TODO:
test_ignore.toml
into it).