Detox #26

hadasz · 2018-10-20T18:52:29Z

Related Issue
Supports Issue #7
Related PRs
This PR is not dependent on any other PR
What does this PR do?
Sets up detox for end to end testing and unit testing modules with substantial native components
Description of Changes
added an e2e folder where e2e test specs can be written.
What gif most accurately describes how I feel towards this PR?
(https://media.giphy.com/media/5Yl08OSg4AckeNHpDc/giphy.gif)

barlock · 2018-10-22T13:55:42Z

The linter is complaining.

e2e/init.js

barlock · 2018-10-23T17:09:56Z

README.md

+
+### End to End Tests
+
+Run `detox build` followed by `detox test` for end to end tests


Can you write an npm script to automate this? just stick detox build && detox test in there.

Also, did travis not work? What broke?

barlock · 2018-10-23T17:10:21Z

e2e/firstTest.spec.js

+  it('should have welcome screen', async () => {
+    await expect(element(by.id('welcome'))).toBeVisible();
+  });
+  /* it('should show hello screen after tap', async () => {


What's the reason for commented code? Looks like boiler plate you didn't delete?

placeholder -- will be deleted in next pull request

e2e/config.json

barlock · 2018-10-23T17:29:47Z

package.json

+    "configurations": {
+      "ios.sim.debug": {
+        "binaryPath": "ios/build/Build/Products/Debug-iphonesimulator/sojourn.app",
+        "build": "cp index.test.js index.js && xcodebuild -project ios/sojourn.xcodeproj -configuration Debug -scheme sojourn -destination id=AC488766-22CC-4F2F-AEE5-F699F8D3BE7A -derivedDataPath ios/build",


I've always used the react-native cli to do builds instead of relying on xcode. What's the difference?

Is the id one specifically on your machine? I'm worried that it isn't portable.

I took out the ID. react-native run-ios starts it's own simulator, so not worth it to use

barlock · 2018-10-23T17:47:34Z

src/utils/meat-grinder.js

+//arguments: file - plaintext file
+//return value:encrypted file
+export async function encryptWithAes(privateKey, plainTextFile) {
+  const iv = 'sixteen bytes iv'; //To DO: randomly generate


How did you plan on using an IV? Per file/user? I think they're optional, though definitely good practice, yeah?

IV will be used per file, so nobody can do cryptanalysis on it.

Do you have any thoughts on how we're going to store the IV? I'm guessing it will need to go in the azure "how to decrypt" file? (Obviously not necessary for this pr, just curious)

barlock · 2018-10-23T17:49:51Z

e2e/init.js

+afterAll(async () => {
+  await adapter.afterAll();
+  console.log('cleaning up');
+  exec('cp index.debug.js index.js');


I don't love the idea of having a completely separate app to do e2e testing. For the short term it's a lovely PoC. What did you have in mind for how this would look in the long run?

I'm a bit confused, the build copies the .test.js and this one copies .debug back over top? Is that a bug?

Is it possible to get the copying of files in the same file? The code distance seems great to me.

I tried doing this with environment variables, but alas, it is not trivial to pass environment variables into a react-native app. One potential work around is dotenv (https://github.com/zetachang/react-native-dotenv) , but you can only have a different .env file for release and debug, so it boils down to the same problem.
Different files is the best thing I can think of right now. Will keep an open mind for possible changes, however.

Move the setup cp into the beforeAll to keep the code distance small

Should this line now just be rm index.js?

I believe your not seeing errors b/c exec is a callback? using fs-extra will fix that.

barlock · 2018-10-23T17:50:28Z

e2e/firstTest.spec.js

+  });
+
+  it('should have welcome screen', async () => {
+    await expect(element(by.id('welcome'))).toBeVisible();


Will this fail if the encryption fails?

this is just a placeholder test

barlock · 2018-10-23T17:51:22Z

src/RCTAesTests.js

+      testPrivateKey,
+      'ffffffffffffffffffffffffffffffffffffffffffffffffffff'
+    );
+    if (!aesOutput) {


When will this ever be falsy? If it fails it looks like it will throw instead of returning an error.

this has been taken out in newest commit

barlock · 2018-10-23T17:54:21Z

.eslintrc.js

  settings: {
    react: {
      pragma: 'React',
      version: '16.5.0'
    }
  },
  rules: {
-    'react/display-name': 'off'
+    'react/display-name': 'off',
+    'no-console': 'off'


I actually like this rule. It usually catches a bunch of debug logic that doesn't need to be there.

Rather than do this, would it make more sense to use inline-ignores where it makes sense?

https://stackoverflow.com/questions/27732209/turning-off-eslint-rule-for-a-specific-line

barlock · 2018-10-25T20:13:59Z

package.json

@@ -9,24 +9,30 @@
    "build:truffle": "truffle compile",
    "ios": "react-native run-ios",
    "lint": "eslint .",
-    "jest": "yarn build:truffle && jest",
+    "jest": "yarn build:truffle && jest src/_tests_",


I've just learned that truffle requires you to use the top level test dir for truffle tests and it isn't changeable.

I'd like to keep the tests alongside the file CA: 7

My reasoning:

When reviewing code it means your test suite and file your testing are next to each other and there's less context switching during a review

git diff shows you the files next to each other and it's a quick personal test to make sure you wrote a test for every file you touched.

I just wanted to be able to specify one file for jest, because it's easier than using transformIgnore options.

I think you're looking for testPathIgnorePatterns

{ testPathIgnorePatterns: [ '/node_modules/', '/e2e/', // this is what you want '/test/', // this will be coming in a pr soon b/c truffle ] }

barlock · 2018-10-25T20:19:50Z

package.json

  },
  "dependencies": {
    "@babel/plugin-proposal-class-properties": "^7.1.0",
+    "@trackforce/react-native-aes-crypto": "^1.2.3",


the "normal" one looks more popular. I think trackforce is doing the thing were they protect themselves from leftpad/eslint situations

barlock · 2018-10-25T20:20:28Z

package.json

@@ -35,9 +41,13 @@
    "babel-core": "7.0.0-bridge.0",
    "babel-eslint": "^10.0.1",
    "babel-jest": "^23.6.0",
+    "browserify": "^12.0.1",


It's new in this commit, remove it?

barlock · 2018-10-25T20:22:22Z

package.json

    "drizzle": "git+https://github.com/trufflesuite/drizzle.git#1.2.3",
    "drizzle-react": "^1.2.0",
    "eth-block-tracker": "^4.0.3",
+    "get-random-values": "^1.1.1",
+    "global": "^4.3.0",


It's a bit easier for me to review dependencies when they're included with the code that's using it. It's also gives better history/thinking. For example, it was pretty easy for me to kill Drizzle sense I could just look at that PR and basically revert it.

Could you remove those that your not using?

barlock · 2018-10-26T14:05:33Z

.travis.yml

 branches:
  only:
-  - master
+    - master
+os: osx


We removed detox, don't need this anymore

barlock · 2018-10-26T14:07:20Z

README.md

+
+### End to End Tests
+
+Run yarn e2e to run end to end tests. Because schemes other than debug and release are not trivial in react-native,


Run `yarn e2e...`

barlock · 2018-10-26T14:13:28Z

e2e/E2ETests.js

+const testPrivateKey =
+  '8238BAE35C77FE4AEBB2DEB1B83A6F0027A01D0E4D93BF5B81F7117796955A17';
+export default class E2ETests extends Component {
+  constructor(props) {


Constructors can't be async so doing async here means you have uncaught exceptions.

The right place is compomnentDidMount

class SomeClass extends Component { state = { input16: 0, input20: 0, input48: 0 }; }

☝️ Class properties mean less typing!

barlock · 2018-10-26T14:15:19Z

e2e/E2ETests.js

+    super(props);
+    this.state = { input16: 0, input20: 0, input48: 0 };
+    inputSizes.map(inputSize => {
+      this.encryptForSize(inputSize);


Best to do this on componentDidMount() for initialization

https://reactjs.org/docs/react-component.html

barlock · 2018-10-26T14:16:25Z

e2e/E2ETests.js

+    }
+    this.setState({ ['input'.concat(sizeInBytes)]: aesOutput.ciphertext });
+    console.warn(this.state);
+    return 'success';


Return seems unnecessary ?

barlock · 2018-10-26T14:21:38Z

e2e/init.js

+afterAll(async () => {
+  await adapter.afterAll();
+  console.log('cleaning up');
+  exec('cp index.debug.js index.js');


Move the setup cp into the beforeAll to keep the code distance small

barlock · 2018-10-26T14:22:48Z

index.ios.js

@@ -0,0 +1,6 @@
+/** @format */
+import './polyfill';


Polyfill was removed in #30

barlock · 2018-10-26T14:22:52Z

index.e2e.js

@@ -0,0 +1,5 @@
+import './polyfill';


Polyfill was removed in #30

barlock · 2018-10-26T14:32:59Z

ios/sojourn/Images.xcassets/AppIcon.appiconset/Contents.json

@@ -1,5 +1,15 @@
 {
  "images": [
+    {
+      "idiom": "iphone",


Are you using these images?

barlock · 2018-10-26T14:35:46Z

package.json

  },
  "dependencies": {
+    "@babel/plugin-proposal-class-properties": "^7.1.0",


Bad merge, this was moved to dev

barlock · 2018-10-26T17:19:53Z

e2e/init.js

+jasmine.getEnv().addReporter(adapter);
+
+beforeAll(async () => {
+  exec('cp index.test.js index.js');


do you mean index.e2e.js?

barlock · 2018-10-26T18:31:59Z

e2e/init.js

+afterAll(async () => {
+  await adapter.afterAll();
+  console.log('cleaning up');
+  exec('cp index.debug.js index.js');


Should this line now just be rm index.js?

barlock · 2018-10-26T18:33:38Z

e2e/init.js

+afterAll(async () => {
+  await adapter.afterAll();
+  console.log('cleaning up');
+  exec('cp index.debug.js index.js');


I believe your not seeing errors b/c exec is a callback? using fs-extra will fix that.

Hadas Zeilberger and others added 9 commits October 10, 2018 13:15

this is a mock for doc purposes

5156b03

implemented aes function

a135e8b

iOS unit tests

bedf211

detox setup

b2397e6

test app for detox

b280036

m

487a645

reset index.js in cleanup of detox tests

7a94375

including e2e test notes in readme

2bd464c

make iv proper length, delete unused files

bf9f553

Hadas Zeilberger and others added 19 commits October 22, 2018 15:30

Merge branch 'master' of github.com:ConsenSys/imagineering-sojourn

500470d

this is a mock for doc purposes

03f3358

implemented aes function

0cdca4e

iOS unit tests

91508b8

detox setup

39919c7

test app for detox

d0402c0

reset index.js in cleanup of detox tests

81aed5b

including e2e test notes in readme

4662b2a

make iv proper length, delete unused files

cbc5eec

this is a mock for doc purposes

4b36be1

implemented aes function

905433d

iOS unit tests

de6824a

detox setup

4540918

make iv proper length, delete unused files

8f4d1c1

package.json

6242961

conflict merge

a8cf21e

detox detox detox

48f5e12

detox

8b5a969

made test directory for unit tests

c065df6

barlock suggested changes Oct 23, 2018

View reviewed changes

Hadas Zeilberger added 12 commits October 24, 2018 09:18

updated index files

a565988

package json alteration to get detox to work in travis

b563268

ensure there is a polyfill

9a8f407

fixed package.json

3cbb5a5

travis <3 react-native

0314527

typo

1855763

install react-native-cli

a92fd4d

travis <3 detox-cli

39eb25d

took detox out of travis

bbf6367

changed some filenames for more clarity

51c3f3c

merge

becab52

updated readme

3defe29

barlock suggested changes Oct 25, 2018

View reviewed changes

barlock suggested changes Oct 26, 2018

View reviewed changes

Hadas Zeilberger added 4 commits October 26, 2018 11:32

code review changes

4abbd18

reverted travis

2d12a1a

made readme more readable

7badc02

post install react-native link"

d1f6e48

barlock suggested changes Oct 26, 2018

View reviewed changes

corrected file name

3ae77a5

barlock suggested changes Oct 26, 2018

View reviewed changes

barlock added 6 commits October 26, 2018 14:41

Merge branch 'master' into detox

7a48c46

Remove Polyfill from index

6830337

Update index.ios.js

772c395

Update meat-grinder.js

69460ca

Update init.js

aa1d73c

Delete index.js

c762ff9

barlock approved these changes Oct 26, 2018

View reviewed changes

barlock merged commit 1f8fb50 into master Oct 26, 2018

barlock deleted the detox branch October 31, 2018 15:07


		### End to End Tests

		Run `detox build` followed by `detox test` for end to end tests


		### End to End Tests

		Run yarn e2e to run end to end tests. Because schemes other than debug and release are not trivial in react-native,

Detox #26

Detox #26

Conversation

hadasz commented Oct 20, 2018

barlock commented Oct 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment