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

Fix Browser CI #3369

Merged
merged 49 commits into from
Jul 3, 2024
Merged

Fix Browser CI #3369

merged 49 commits into from
Jul 3, 2024

Conversation

acolytec3
Copy link
Contributor

@acolytec3 acolytec3 commented Apr 24, 2024

Fixes browser tests

  • Update vitest to 2.0 beta 12
  • Remove deprecated hardhat CI job file
  • Adds various test config edits to account for tests that can't run in browser

@acolytec3 acolytec3 added the dependencies Pull requests that update a dependency file label Apr 24, 2024
import { nodePolyfills } from 'vite-plugin-node-polyfills'
import wasm from 'vite-plugin-wasm'

const config = defineConfig({
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
const config = defineConfig({
// https://vitest.dev/config/
const config = defineConfig({

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@roninjin10 do you have any insights on why an arbitrary browser test script seems to always fail on CI? All of our browser tests pass locally but when they run on CI, we get these failed to resolve dynamically imported module... errors. I've tried all manner of config settings trying to remove parallel test runs and isolated environments, etc, with no success. At this point, I haven't come up with a reproducible "minimal" build to point the vitest devs at so have hesitated to open an issue there (though may do so soon)

Copy link
Collaborator

Choose a reason for hiding this comment

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

@acolytec3 try setting VITEST_BROWSER_DEBUG=true environment variable

Copy link
Collaborator

Choose a reason for hiding this comment

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

Btw if the debug logging doesn't provide any insight let me know I'd be happy to look at this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It just keeps doing this sort of thing - https://github.com/ethereumjs/ethereumjs-monorepo/actions/runs/8943487337/job/24568309037#step:9:74

Debug logging doesn't seem to tell me anything more. It's not always the tx test run that fails either. I can comment out tx and another one will fail the same way (or I can tell vitest to skip the one test file that's not being loaded and it will still fail on tx with a different file having no tests fail). Very weird.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems to have started after vitest v1.5.1 was released

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you don't have a quick instinct on it, I'll just open an issue on vitest and see if they're willing to look at our vitest config since I don't know how else to reproduce it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

. It's not always the tx test run that fails either. I can comment out tx and another one will fail the same way (or I can tell vitest to skip the one test file that's not being loaded and it will still fail on tx with a different file having no tests fail). Very weird.

oh that is really wierd and definitely smells like an internal race condition bug in vitest

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was my instinct as well. Will report it and see if they're willing to help.

@holgerd77
Copy link
Member

Have restarted CI since blockchain and state tests were failing (a bit unexpectedly + couldn't get anything out of the logs)

@acolytec3 acolytec3 changed the title Update vitest Fix Browser CI Jul 3, 2024
Copy link
Member

@holgerd77 holgerd77 left a comment

Choose a reason for hiding this comment

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

LGTM, great! 🎉

- run: npm run test:browser -w=@ethereumjs/evm
- run: npm run test:browser -w=@ethereumjs/vm
- run: npm run test:browser -w=@ethereumjs/verkle
- run: npm run test:browser --workspaces --if-present
Copy link
Member

Choose a reason for hiding this comment

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

🙏

@@ -1,51 +0,0 @@
name: E2E Hardhat Tests
Copy link
Member

Choose a reason for hiding this comment

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

Hardhat out, yeah, guess that makes sense.

Not so realistic we'll give this so much attention again in the future.

exclude: [
...configDefaults.exclude,
// readDirSync method not provided fs mock for vite
'test/precompiles/eip-2537-bls.spec.ts',
Copy link
Member

Choose a reason for hiding this comment

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

Guess this can be removed again (?) after BLS merge

Copy link
Member

Choose a reason for hiding this comment

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

Update: just tested and realized it's another thing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, there are probably several of these where the mock that vitest injects a polyfill that's not fully mapping to the node builtin we're referencing.

@holgerd77 holgerd77 merged commit cc2848a into master Jul 3, 2024
34 checks passed
@holgerd77 holgerd77 deleted the vitest-update branch July 3, 2024 10:57
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.

vitest type issue (WeakKey type)
3 participants