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

Add State Manager API tests to karma #653

Merged
merged 1 commit into from
Jan 29, 2020
Merged

Conversation

ryanio
Copy link
Contributor

@ryanio ryanio commented Jan 22, 2020

This PR:

  1. Adds tests/api/state/stateManager.js back to karma.
  2. Skips slow test when running in karma: should generate the genesis state root correctly for mainnet
  3. Modifies test should generate correct genesis state root for all chains to should generate the genesis state root correctly for all other chains (since mainnet is already tested prior).
  4. Runs karma in both headless Firefox and Chrome for increased browser coverage.
  5. Removes unneeded karma-detect-browsers.

Closes #465

@ryanio
Copy link
Contributor Author

ryanio commented Jan 22, 2020

Sorry about the prettified files, the diffs are a bit hard to read/parse.

@ryanio ryanio force-pushed the addAPIStateManagerTestsToKarma branch from ebee3f9 to 13b25f3 Compare January 22, 2020 18:37
@ryanio ryanio requested review from s1na and holgerd77 January 22, 2020 18:42
@codecov
Copy link

codecov bot commented Jan 22, 2020

Codecov Report

Merging #653 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #653   +/-   ##
======================================
  Coverage    91.4%   91.4%           
======================================
  Files          31      31           
  Lines        1978    1978           
  Branches      326     326           
======================================
  Hits         1808    1808           
  Misses         90      90           
  Partials       80      80
Flag Coverage Δ
#vm 91.4% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f363377...1dd35f6. Read the comment docs.

Copy link
Contributor

@evertonfraga evertonfraga left a comment

Choose a reason for hiding this comment

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

That's great, @ryanio.

I have only one suggestion, which is to extract the expression typeof window !== 'undefined' && window.__karma__ to a function in util.js.

Not that we want to reuse that much around, but just to give this logic a proper name.

exclude: [
'./tests/api/state/stateManager.js', // 4, "# should clear the cache when the state root is set"
],
exclude: [],
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@ryanio ryanio force-pushed the addAPIStateManagerTestsToKarma branch from 13b25f3 to 1dd35f6 Compare January 22, 2020 21:08
Copy link
Contributor

@evertonfraga evertonfraga left a comment

Choose a reason for hiding this comment

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

Logic was extracted to isRunningInKarma().


st.equal(stateRoot.toString('hex'), genesisData.genesis_state_root)
t.test('should generate the genesis state root correctly for mainnet', async st => {
if (isRunningInKarma()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

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.

Yes, can confirm that this is hard to read/review 😛, next time stuff like this should better be two subsequent PRs, otherwise the structural risk to overlook something is unnecessarily increased.

Otherwise looks good. Won't merge myself, not sure if this intervenes somehow with the monorepo process or something, but otherwise @evertonfraga or @ryanio feel free to merge.

This is increased browser (type) coverage is also cool, browser compatibility gets historically somewhat too little attention within EthereumJS.

@ryanio
Copy link
Contributor Author

ryanio commented Jan 27, 2020

Thanks, sounds good. In the future if a file isn't prettified I'll leave it out so the diff is cleaner and run the prettify in a separate PR.

@ryanio ryanio merged commit 15dabb7 into master Jan 29, 2020
@evertonfraga evertonfraga deleted the addAPIStateManagerTestsToKarma branch April 16, 2020 11:26
@evertonfraga evertonfraga added this to the VM v5 milestone Jun 12, 2020
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.

Improve VM browser compatibilty / test coverage
3 participants