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

chore: upgrade to node 11 and fix all failing unit tests #1075

Merged
merged 12 commits into from Feb 20, 2019

Conversation

@luciorubeens
Copy link
Member

commented Feb 13, 2019

Proposed changes

  • Fix all failing unit tests
  • Upgrade to node 11 and used the Intl polyfill to properly run tests
  • Refactor e2e tests and add more specs

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • Refactoring (improve a current implementation without adding a new feature or fixing a bug)
  • Build (changes that affect the build system)
  • Docs (documentation only changes)
  • Test (adding missing tests or fixing existing tests)

Checklist

  • I have read the CONTRIBUTING documentation
  • Lint and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
@ArkEcosystemBot

This comment has been minimized.

Copy link
Member

commented Feb 13, 2019

@alexbarnsley @j-a-m-l - please review this in the next few days. Be sure to explicitly select labels so I know what's going on.

If no reviewer appears after a week, a reminder will be sent out.

@codecov-io

This comment has been minimized.

Copy link

commented Feb 13, 2019

Codecov Report

Merging #1075 into develop will increase coverage by 0.08%.
The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           develop   #1075      +/-   ##
==========================================
+ Coverage    39.62%   39.7%   +0.08%     
==========================================
  Files          205     205              
  Lines         5164    5163       -1     
  Branches      1013    1019       +6     
==========================================
+ Hits          2046    2050       +4     
+ Misses        2998    2993       -5     
  Partials       120     120
Impacted Files Coverage Δ
src/renderer/components/App/AppWelcome.vue 0% <ø> (ø) ⬆️
src/renderer/components/App/AppIntroScreen.vue 0% <0%> (ø) ⬆️
.../renderer/store/plugins/vuex-persist-migrations.js 95.65% <50%> (-4.35%) ⬇️
src/renderer/components/utils/TableWrapper.js 0% <0%> (-25%) ⬇️
...ponents/Wallet/WalletDelegates/WalletDelegates.vue 38.29% <0%> (-10.64%) ⬇️
src/renderer/store/modules/peer.js 69.15% <0%> (-3.28%) ⬇️
src/renderer/store/modules/transaction.js 50.7% <0%> (-1.41%) ⬇️
src/renderer/store/modules/session.js 35.84% <0%> (-0.95%) ⬇️
src/renderer/services/client.js 69.89% <0%> (+2.67%) ⬆️
...erer/components/Menu/MenuDropdown/MenuDropdown.vue 91.66% <0%> (+4.16%) ⬆️
... and 5 more

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 a7fddee...cb36394. Read the comment docs.

@luciorubeens luciorubeens referenced this pull request Feb 13, 2019
5 of 7 tasks complete

@luciorubeens luciorubeens requested a review from faustbrian as a code owner Feb 19, 2019

@j-a-m-l
Copy link
Contributor

left a comment

Apart from the comment, everything looks fine.

@ArkEcosystemBot

This comment has been minimized.

Copy link
Member

commented Feb 19, 2019

@luciorubeens Your pull request needs some changes. Please wait for a comment from one of our developers for more information.

@@ -153,11 +153,11 @@ describe('Mixins > Currency', () => {
it('should work with big quantities', () => {
let amount = Math.pow(10, 12) + 0.01

expect(format(amount, { currencyFrom: 'network' })).toEqual('× 1,000,000,000,000.01')
expect(format(amount, { currencyFrom: 'network' })).toEqual('× 1,000,000,000,000.00999424')

This comment has been minimized.

Copy link
@j-a-m-l

j-a-m-l Feb 19, 2019

Contributor

What could be the reason to this? I mean, did something change on Node 11? If that's the case we should try to fix this behaviour or, at least, document it.

This comment has been minimized.

Copy link
@luciorubeens

luciorubeens Feb 19, 2019

Author Member

It's related to the Intl polyfill:

Node.js 0.12 has the Intl APIs built-in, but only includes the English locale data by default

Probably a precision error, but this polyfill is used only for the tests, in the browser will be used the native api, returning the correct data.

You added a note related some lines above: https://github.com/ArkEcosystem/desktop-wallet/blob/develop/__tests__/unit/mixins/currency.spec.js#L77

This comment has been minimized.

Copy link
@j-a-m-l

j-a-m-l Feb 20, 2019

Contributor

That comment was about the format: since Node was including English only, it formats every currency the same way.

@j-a-m-l
Copy link
Contributor

left a comment

I'm going to approve the PR and create a new one with some related and unrelated small changes, to not bloat more this one.

@j-a-m-l j-a-m-l merged commit f3069aa into develop Feb 20, 2019

1 check passed

ci/circleci: test-node-11 Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.