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 test issues #468

Merged
merged 4 commits into from Mar 15, 2019
Merged

Fix browser test issues #468

merged 4 commits into from Mar 15, 2019

Conversation

s1na
Copy link
Contributor

@s1na s1na commented Mar 11, 2019

See #465.

@coveralls
Copy link

coveralls commented Mar 11, 2019

Coverage Status

Coverage remained the same at 95.137% when pulling 847a8f3 on fix/browser-tests into 469c1aa on master.

@holgerd77
Copy link
Member

Rebased this.

@holgerd77
Copy link
Member

@s1na I've now fixed the index and blockchain test timeout issues by running against Goerli, do you think this is a legit fix? Can you generally review the code parts I added?

@holgerd77
Copy link
Member

Because I was interested, results from coverage report just on the API tests:

Bildschirmfoto 2019-03-14 um 23 29 32

So this is a start, but not there yet either.

@s1na
Copy link
Contributor Author

s1na commented Mar 15, 2019

@holgerd77 that was a really interesting idea!

  1. Does that fix the timeout because the goerli genesis state is smaller?
  2. How does the coverage of API tests on goerli and mainnet compare? are they different?
  3. I meant to also try to find why it is that generating canonical genesis is taking so much longer on browser than on node. Is that because of limited resources in the browser, or maybe that the database has a different implementation or different characteristics on browser? 🤔

@holgerd77
Copy link
Member

  1. Yes
  2. Coverage shouldn't be different, I also had the thought while working on this though that we at some point should test different chains (minimally: 2) within the API tests. I would suggest that we know stick to the Goerli version, eventually along the investigation you mentioned in 3) (makes very-much sense, yes, a bit strange the drop in performance and likely also has real-world implications for library usage) we can go more broadly. Does this make sense?

@s1na
Copy link
Contributor Author

s1na commented Mar 15, 2019

we at some point should test different chains

I was just about to write the same!

Yes, that plan sounds good.

@holgerd77
Copy link
Member

😀

Can you do an explicit short review of my code changes?

@s1na
Copy link
Contributor Author

s1na commented Mar 15, 2019

The changes look good, but I can't approve as I'm the PR author 😄

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.

Thanks, then I will approve and merge here.

@holgerd77 holgerd77 merged commit e7bb009 into master Mar 15, 2019
@s1na s1na deleted the fix/browser-tests branch March 15, 2019 11:56
@holgerd77 holgerd77 changed the title [WIP] Fix browser test issues Fix browser test issues Mar 22, 2019
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

4 participants