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

Generalized Runtime Docs / (First round) Bun Support #3219

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

holgerd77
Copy link
Member

This PR adds a generalized "Runtimes" information section to the docs combining info for Node.js and browser (rewritten to be more specific) and then also adds a new "Bun" sub section there (we might want to add Deno as well).

I've done this in an examplaric way for one library, open for a first round of feedback, then I would expand (the same section/structure) to the other libraries (except client).

I've also added a simple.ts example, I would want to have one for each package, since this just makes sense in general.

For Bun I have also added a new GH Actions script. This is meant to be run nightly. After a first round I would comment out all non-working packages. For the working ones we can add the new "Bun Support" runtime section.

Copy link

codecov bot commented Jan 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (11f3a9c) 86.77% compared to head (43f245a) 86.85%.

Additional details and impacted files

Impacted file tree graph

Flag Coverage Δ
block 88.33% <ø> (ø)
blockchain 91.61% <ø> (ø)
client 84.69% <ø> (ø)
common 98.25% <ø> (ø)
devp2p 82.12% <ø> (ø)
ethash ∅ <ø> (∅)
evm 74.33% <ø> (ø)
genesis 99.98% <ø> (ø)
rlp ∅ <ø> (∅)
statemanager 75.86% <ø> (ø)
trie 89.16% <ø> (ø)
tx 95.45% <ø> (ø)
util 89.13% <ø> (?)
vm 80.20% <ø> (ø)
wallet 88.35% <ø> (ø)

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

Copy link
Collaborator

@roninjin10 roninjin10 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 very nice to see this happening! I've been implicitly using bun in tevm for the evm blockchain and state packages for a while without issues

- uses: actions/checkout@v3
with:
submodules: recursive
- uses: oven-sh/setup-bun@v1
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- uses: oven-sh/setup-bun@v1
- uses: oven-sh/setup-bun@v1
with:
bun-version: 1.0.24

Might want to pin the version so there is no ambiguity if a pr caused a regression or bun version changed it.

Alternatively consider just echoing the bun version in the next step of this test so if you are debugging a workflow you know exactly which version of bun it used

# working-directory: packages/client

- name: Test Common
run: bunx vitest run test
Copy link
Collaborator

@roninjin10 roninjin10 Feb 9, 2024

Choose a reason for hiding this comment

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

I don't think this is actually using bun. Because vitest is an executable bun should default to using node here. I think you need to pass in the --bun flag to use bun and you might have issues related more to vitest than bun.

Suggested change
run: bunx vitest run test
run: bun run --bun vitest run test

Copy link
Collaborator

Choose a reason for hiding this comment

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

Docs on this are here https://bun.sh/docs/cli/run#bun

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, that is the "tip of the year"! 🙏 🙂

I was already wondering why things are so smooth. 😂 That is the missing link I needed!

Copy link
Member Author

Choose a reason for hiding this comment

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

Just to note that this is not yet working (using bun 1.0.26). Tested with common (counter test with the original version worked), new version just hangs before any output.

Not dropping here for you to debug, might just be our homework to do with the bun updates adjusting our code base. But just to drop the information here.

@holgerd77
Copy link
Member Author

@roninjin10 Thanks a lot for the feedback, that helps a lot!

I wanted to wait for our "self-contained examples" work (see some other now merged in PRs), so we basically have made all examples self-contained, so that they can now run "on their own" with all necessary dependencies included and stuff, the examples are now also included in a dedicated examples folder per package.

So this makes bun EthereumJS API testing easier. The examples work is now done, so I will pick up this PR sometime soon (next week or the week after I guess). Sufficient basis for this now, also with your --bun flag tip (thanks again for that!).

@holgerd77
Copy link
Member Author

Resolved merge conflict here and updated this via UI

@holgerd77
Copy link
Member Author

Update: gave this another try.

There is an issue open on Vitest support over on Bun which is still open: oven-sh/bun#4145 and states that there is something in Bun still missing for this to work:

grafik

So seems it is still too early to try on this route.

@holgerd77
Copy link
Member Author

What was an interesting experiment though: I managed to switch over our transaction test runner (which takes quite some time, >30 sec) to use the Bun native test runner (see docs) by doing some few replacements:

// import { assert, describe, it } from 'vitest'
import { describe, expect, test } from 'bun:test'

// assert.ok(!txIsValid, `Transaction should be invalid on ${forkName}`)
expect(!txIsValid).toBeTruthy()

// assert.ok(hashAndSenderAreCorrect && txIsValid,`Transaction should be valid on ${forkName}`)
expect(hashAndSenderAreCorrect && txIsValid).toBeTruthy()

// assert.ok(shouldBeInvalid, `Transaction should be invalid on ${forkName}`)
expect(shouldBeInvalid).toBeTruthy()

//assert.fail(`Transaction should be valid on ${forkName}`) (no replacement)

This produces the following output:

grafik

Test execution times are 31.11s with Bun and 32.97s with Vitest.

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.

None yet

2 participants