repo: update vitest

[jochem-brouwer]

Vitest has published their first non-beta release a few days ago: https://www.npmjs.com/package/vitest/v/1.0.0

This PR updates vitest. It seems like the browser tests hang 🤔

Note: also a huge impact on package-lock, should definitely be reviewed (what are these inserted dev and peer fields?)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (4b358b4) 88.29% compared to head (13a1356) 84.97%.

Additional details and impacted files

Flag	Coverage Δ
block	82.25% <ø> (-5.36%)	⬇️
blockchain	90.91% <ø> (-0.70%)	⬇️
client	84.88% <ø> (+0.31%)	⬆️
common	94.25% <ø> (-4.01%)	⬇️
devp2p	81.59% <ø> (?)
ethash	∅ <ø> (∅)
evm	76.62% <ø> (-0.30%)	⬇️
genesis	99.98% <ø> (ø)
rlp	?
statemanager	86.57% <ø> (ø)
trie	62.93% <ø> (-26.46%)	⬇️
tx	91.00% <ø> (-4.89%)	⬇️
util	86.93% <ø> (-2.20%)	⬇️
vm	63.61% <ø> (-16.66%)	⬇️
wallet	90.98% <100.00%> (-0.03%)	⬇️

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

[holgerd77]

What is the status here? @ScottyPoi can you give a summary what changes are necessary to get this working? Everything "expected" or some unexpected stuff here?

[ScottyPoi]

@holgerd77 the main issue here in the browsers is with core node modules. especially anywhere we use EventEmitters.

Using a plugin called vite-plugin-node-polyfill is the recommended solution, which works for tests that do not directly use an event emitter, but does not fix tests that use things like Common or AsyncEventEmitter which are extensions of the node EventEmitter class.

This is the current error from those tests:

SyntaxError: The requested module '.../node_modules/events/events.js?v=09391bae' does not provide an export named 'EventEmitter'

[ScottyPoi]

😬
been working on this all day and can't seem to solve the EventEmitter thing, with polyfills or otherwise. I will poke at it some more tomorrow.

[ScottyPoi]

the wallet test:node passes locally. not sure why they timeout on here

works now 👍

[ScottyPoi]

https://github.com/vitest-dev/vitest/releases

They released v1.2 today which includes some fixes to browser mode.
Going to upgrade with fingers crossed that something in there is helpful to me!

[ScottyPoi]

Browser Tests Pass!

Using this as the base config for browser mode handles most issues:

defineConfig({
  test: {
    browser: {
      enabled: true,
      name: 'chrome',
      headless: true,
    },
  },
  resolve: {
    alias: {
      events: 'eventemitter3',
    },
  },
  plugins: [
    nodePolyfills({
      include: ['util', 'fs', 'buffer'],
    }),
  ],
})

Additional configs in individual packages handle any specific extra requirements.

[jochem-brouwer]

Wow the CI passes, super great! I will give this a review tomorrow!

[ScottyPoi]

I'm trying to figure out why coverage went down on certain files, especially in /client. I think it has to do with some await import(...) statements, or some of the vi.mock things that we do in some tests.

[ScottyPoi]

Browser-tests passed in dc8b4f6

I don't know why they aren't passing now. The test that fails on here passes locally

[ScottyPoi]

Update:

• The only blocker from this being fully passing is browser / block, which fails on github, but passes locally.
• I started working out the bugs in the browser / VM tests. and have 22/28 test files passing.
  • VM tests are not a part of the github CI "test-all-browser" workflow.
  • I believe I have identified some bugs and false positives in the VM tests. I'd have to dig a bit more to know whether they were valid tests before the vitest upgrade or not, so I don't know if those fixes fall in the scope of this PR, or whether to finish that work separately.
  • For now I will continue working on it on this branch, but won't push those changes for now, because it will be a lot of additional commits

[jochem-brouwer]

Ah this looks similar to what we had a few weeks back with pinning the vitest dependency. @acolytec3 could you maybe take a look here?

[ScottyPoi]

Ah this looks similar to what we had a few weeks back with pinning the vitest dependency. @acolytec3 could you maybe take a look here?

what do you mean by "pinning"?

[acolytec3]

Ah this looks similar to what we had a few weeks back with pinning the vitest dependency. @acolytec3 could you maybe take a look here?

what do you mean by "pinning"?

Just putting the dependency to a specific version in package.json rather than ^0.2.0

[jochem-brouwer]

Ah pinning might not have been the exact right term, I mean changes introduced in for instance this commit: fe1a1dc (note: from the tsx PR)

[acolytec3]

Should we add this elsewhere?

[ScottyPoi]

I'm going to do some cleanup on this branch. some of these config edits might be unnecessary.

[acolytec3]

Sounds good.

[acolytec3]

This looks great and I can verify that everything is working correctly. Left one comment on tsconfig (though I think it could be addressed in a future PR.

[ScottyPoi]

Cool, thanks!
I'm still bothered by the codecov/project stats going down. Can make that a separate issue, though, i suppose.

[ScottyPoi]

Update:

Still around and around with the same issue today. Not sure what else to try.

The codecov failure --

There is a mismatch between the number of uploaded coverage reports between the base and head being compared. The documentation for Codecov mentions this as a reason for unexpected results, but offers no explicit solution.

The test-all-browsers failure --

The results are inconsistent. All tests pass locally, with the same environment setup and everything. Sometimes all the tests will run and pass. Most of the time, either block or blockchain throws an import error which stops the tests, and often the same test will then pass on rerun.

I have tried various solutions, including editing the workflow file, but have not escaped this particular issue, nor figured out an explanation for why this might happen.

One possibility I have not explored is perhaps running all of the browser tests in one workflow causes an issue with resource constraints in github actions. We could try breaking the browser tests up into more than one workflow.

[holgerd77]

Maybe put this aside until our call on Wednesday and we can talk some things through there.

[holgerd77]

Will close here, since this PR has gotten too extensive.

Scotty has already extracted some pre-work to #3266, we can continue/pick up this work here at some point in #3281.

Note that it will likely nevertheless be useful to come back to this PR for inspiration/guidance for changes, eventually it might also still make sense to cherry-pick selected commits.