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

Lazily download test262 repository #3214

Merged
merged 4 commits into from
Sep 21, 2023
Merged

Lazily download test262 repository #3214

merged 4 commits into from
Sep 21, 2023

Conversation

HalidOdat
Copy link
Member

@HalidOdat HalidOdat commented Aug 14, 2023

This Pull Request fixes/closes #3095 .

This PR makes the downloads the test262 repository, if we run the boa_tester with run 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:

  • The commit is hard coded, add a config file for test262 default commit (maybe merge test_ignore.toml into it).
    • Add ability to specify commit on test262 clone, via CLI commands
    • Warn user that the current commit is older than test262 HEAD
  • Better error reporting

@HalidOdat HalidOdat added the enhancement New feature or request label Aug 14, 2023
boa_tester/src/main.rs Outdated Show resolved Hide resolved
@jasonwilliams
Copy link
Member

Good idea, I don't see there being an issue with this. It should make the whole git submodules issue easier

Copy link

@gakonst gakonst left a 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 ^^

boa_tester/src/main.rs Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Aug 22, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.09% ⚠️

Comparison is base (8a78061) 50.31% compared to head (55215e5) 50.23%.
Report is 2 commits behind head on main.

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     
Files Changed Coverage Δ
boa_tester/src/main.rs 0.00% <0.00%> (ø)
boa_tester/src/results.rs 0.00% <0.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@HalidOdat HalidOdat force-pushed the lazy-test262-download branch 2 times, most recently from f1afff0 to 08a6203 Compare August 22, 2023 19:54
@github-actions
Copy link

github-actions bot commented Aug 22, 2023

Test262 conformance changes

Test result main count PR count difference
Total 95,574 95,574 0
Passed 75,084 75,084 0
Ignored 19,494 19,494 0
Failed 996 996 0
Panics 0 0 0
Conformance 78.56% 78.56% 0.00%

@HalidOdat HalidOdat requested a review from a team August 22, 2023 21:52
@HalidOdat
Copy link
Member Author

HalidOdat commented Aug 22, 2023

This is now complete, and ready for review/merge :)

Added --test262-update for automatically updating the local repository and --test262-commit for specifying a commit.

Now tester warns if local test262 clone is out of sync with origin/main:

Warning Test262 repository is not in sync, use '--test262-update' to automatically update it:
    Current commit: bdddd9e2d286600462958c7bac6af10a2946134b Update test for monthDayFromFields()
    Latest commit:  04a84fc5fddbc2498d344cc5b85dd6bf6d76c96e Fix test/intl402/DurationFormat/prototype/format/style-digital-en.js

@jedel1043
Copy link
Member

jedel1043 commented Aug 23, 2023

Did you consider using git2 instead of calling manual commands? Maybe that could further simplify the repo handling logic.

EDIT: There's also gix which is implemented in Rust only.

@HalidOdat
Copy link
Member Author

HalidOdat commented Aug 23, 2023

Did you consider using git2 instead of calling manual commands?

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.

@gakonst
Copy link

gakonst commented Sep 7, 2023

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.

Copy link
Member

@jedel1043 jedel1043 left a 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?

@HalidOdat
Copy link
Member Author

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 test_ignore.toml file into something like test262_config.toml, to avoid having more than one config file boa_tester

@jedel1043
Copy link
Member

Maybe merge test_ignore.toml file into something like test262_config.toml, to avoid having more than one config file boa_tester

Great idea! We just have to create a new ignore section and we can put a commit property on the root

@jasonwilliams
Copy link
Member

Hey @gakonst what does Reth use Boa for? I’m curious

@HalidOdat
Copy link
Member Author

Maybe merge test_ignore.toml file into something like test262_config.toml, to avoid having more than one config file boa_tester

Great idea! We just have to create a new ignore section and we can put a commit property on the root

Implemented in the latest commit 3a0d75c :)

@gakonst
Copy link

gakonst commented Sep 21, 2023

Hey @gakonst what does Reth use Boa for? I’m curious

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.

Copy link
Member

@jedel1043 jedel1043 left a 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 :)

boa_tester/src/main.rs Outdated Show resolved Hide resolved
@HalidOdat HalidOdat requested review from a team and removed request for gakonst September 21, 2023 20:21
Copy link
Member

@nekevss nekevss left a 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!

@HalidOdat HalidOdat added this pull request to the merge queue Sep 21, 2023
Merged via the queue into main with commit 749a43f Sep 21, 2023
9 checks passed
@HalidOdat HalidOdat deleted the lazy-test262-download branch September 21, 2023 23:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider removing tc39/test262 submodule and download lazily
6 participants