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 e2e on CI #1537

Merged
merged 23 commits into from
Nov 2, 2019
Merged

Fix e2e on CI #1537

merged 23 commits into from
Nov 2, 2019

Conversation

martinvol
Copy link
Contributor

@martinvol martinvol commented Oct 30, 2019

Description

Tested

itself.

Other changes

Deleted legacy scripts related to e2e.

Related issues

Backwards compatibility

@codecov
Copy link

codecov bot commented Oct 30, 2019

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff            @@
##           master    #1537    +/-   ##
========================================
  Coverage   73.61%   73.61%            
========================================
  Files         277      277            
  Lines        7443     7443            
  Branches      956      662   -294     
========================================
  Hits         5479     5479            
  Misses       1852     1852            
  Partials      112      112
Flag Coverage Δ
#mobile 73.61% <ø> (ø) ⬆️

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 82aac04...fd784fc. Read the comment docs.

@martinvol martinvol force-pushed the martinvol/small_e2e_fix branch from 954aa10 to c10929a Compare October 31, 2019 18:14
@martinvol martinvol marked this pull request as ready for review October 31, 2019 18:21
@@ -202,8 +202,8 @@ jobs:
# echo "Cache found, just run post-script."
# yarn postinstall
# fi
yarn
yarn build
yarn || yarn
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any chance we can figure out why the yarn command is flaky? Retries will only get us so far and increase the time to CI reporting real failures. I understand if you don't have time now, but creating an issue with what you know and a TODO here pointing to that issue would be helpful.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, so are the failures because of wix/Detox#1723? if so a reference inline to this file would be good

Copy link
Contributor Author

@martinvol martinvol Nov 1, 2019

Choose a reason for hiding this comment

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

We do those retries in many places, for example here:

yarn install || yarn install

It is because because sometimes npm has a latency problem and the whole build fails. The retry is only executed if the jobs fails. Also the build command, as it requires network resources it's not guaranteed it's 100% deterministic.

The problems related to detox is in Run test itself, and it has the inline reference indeed :)

@martinvol martinvol force-pushed the martinvol/small_e2e_fix branch from d2042c1 to 228935e Compare November 1, 2019 16:23
@martinvol martinvol force-pushed the martinvol/small_e2e_fix branch from 228935e to 7c0e523 Compare November 1, 2019 17:05
@martinvol martinvol force-pushed the martinvol/small_e2e_fix branch from 7c0e523 to 5137d1d Compare November 1, 2019 21:11
@martinvol martinvol added the automerge Have PR merge automatically when checks pass label Nov 2, 2019
@celo-ci-bot-user celo-ci-bot-user merged commit 8848975 into master Nov 2, 2019
@celo-ci-bot-user celo-ci-bot-user deleted the martinvol/small_e2e_fix branch November 2, 2019 00:56
aaronmgdr added a commit that referenced this pull request Nov 2, 2019
* master: (62 commits)
  Fix e2e on CI (#1537)
  Allow a specified address to disable/enable the Exchange  (#1467)
  Avoid re-encrypting key files with yarn keys:encrypt command (#1560)
  Support protocol hotfixing (#613)
  Point e2e tests back (#1562)
  Refactor to Accounts.sol (#1392)
  Add selectIssuers Transaction (#1327)
  [Wallet] Get React Native Hot Reloading Working (#1551)
  Unify to prefix messages for signing (#1473)
  [Wallet] Improve error handling around account creation and keystore ops (#1497)
  Add CI test for checking licenses and misc package.json cleanup (#1550)
  [Wallet] Implement SMS invite on iOS (#1541)
  CI: brings back to master (#1554)
  Validators: uses Ethereum address for proof of possession (#1494)
  Validate Attestation Requests (#1468)
  Rename hosted node references to forno (#1511)
  Bump rubyzip from 1.2.3 to 1.3.0 in /packages/mobile (#1508)
  Added txpool family to geth apis. Sorted geth cmd options (#1462)
  [Wallet] Fix yarn dev command for running android (#1534)
  [Wallet] iOS info plist changes and version bump (#1539)
  ...

# Conflicts:
#	yarn.lock
aaronmgdr added a commit that referenced this pull request Dec 5, 2019
* master: (73 commits)
  Fix Ethstats Image reference (#1577)
  EU Cookies Behavior Change (#1447)
  [verifier] Upgrade to RN 61 (#1572)
  [Wallet] Update link styles and Implement VerificationEducationScreen (#1565)
  [wallet] Added native phone picker (#1310)
  [Wallet] Set up new verification screen skeletons (#1563)
  Bump e2e test migrate numbers where needed (#1567)
  [Wallet] Create new carousel component (#1555)
  [Wallet] Protect Backup Key and Safeguards with PIN (#1556)
  Increase ganache gas limit (#1569)
  Re-work locked gold requirements for validators and groups (#1474)
  Fix e2e on CI (#1537)
  Allow a specified address to disable/enable the Exchange  (#1467)
  Avoid re-encrypting key files with yarn keys:encrypt command (#1560)
  Support protocol hotfixing (#613)
  Point e2e tests back (#1562)
  Refactor to Accounts.sol (#1392)
  Add selectIssuers Transaction (#1327)
  [Wallet] Get React Native Hot Reloading Working (#1551)
  Unify to prefix messages for signing (#1473)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Have PR merge automatically when checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants