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

Common, Blockchain: add browser tests #1380

Merged
merged 6 commits into from Jul 29, 2021

Conversation

emersonmacro
Copy link
Contributor

Closes #1338

@codecov
Copy link

codecov bot commented Jul 29, 2021

Codecov Report

Merging #1380 (0cb1b9b) into master (755c4a6) will increase coverage by 0.17%.
The diff coverage is n/a.

Impacted file tree graph

Flag Coverage Δ
block 86.06% <ø> (+0.15%) ⬆️
blockchain 83.49% <ø> (ø)
client 84.20% <ø> (+0.65%) ⬆️
common 93.02% <ø> (ø)
devp2p 83.56% <ø> (+0.07%) ⬆️
ethash 82.83% <ø> (ø)
tx 88.24% <ø> (-0.12%) ⬇️
vm 79.32% <ø> (ø)

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

@emersonmacro
Copy link
Contributor Author

@ryanio I took a stab at #1338. Build processes are definitely not my specialty - I got a setup working in common, but a similar setup is failing for blockchain. The files are there and it's entering the first test, but it gets stuck at this line and times out https://github.com/ethereumjs/ethereumjs-monorepo/blob/master/packages/blockchain/test/clique.spec.ts#L13. Wondering if either A) you might have an idea of why it's not working for the blockchain tests or B) maybe we just add browser tests for common now and handle blockchain separately. Or I guess C) I keep digging on why it's not working for blockchain. Any thoughts?

Also, I got a linting error on code I didn't touch - it was complaining that Partial was not defined. Have you seen that before? I added an ignore comment just so I could get this WIP PR up, but wouldn't want to merge that if there's a real fix available. https://github.com/ethereumjs/ethereumjs-monorepo/pull/1380/files#diff-970bb4d849f8275a7e4a4d17f71211e3a99fac7e27a9f3c0b826746287135ddbR169

@ryanio
Copy link
Contributor

ryanio commented Jul 29, 2021

@emersonmacro thanks for getting started on this!

i am going to switch from browserify to karma-typescript since that is a more modern setup that doesn't require a build step. i will also convert block and tx from browserify to karma-typescript.

seems to all be passing for me locally, let's see how ci does.

@ryanio
Copy link
Contributor

ryanio commented Jul 29, 2021

Also, I got a linting error on code I didn't touch - it was complaining that Partial was not defined. Have you seen that before? I added an ignore comment just so I could get this WIP PR up, but wouldn't want to merge that if there's a real fix available.

haven't seen that before, and didn't get any errors locally when i removed the newly added ignores for Partial, i will see if it catches on ci though.

Copy link
Contributor

@ryanio ryanio left a comment

Choose a reason for hiding this comment

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

lgtm!

@ryanio ryanio merged commit 25ec1ac into master Jul 29, 2021
@ryanio ryanio deleted the common-blockchain-add-browser-tests branch July 29, 2021 17:41
@emersonmacro
Copy link
Contributor Author

@ryanio thanks!

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.

Common/Blockchain: add browser tests
2 participants