App logs #200
App logs #200
Conversation
|
|
||
| it(`the constructor should catch exception from ArtifactsPathsProvider`, async () => { | ||
| mockCommandLineArgs({'artifacts-location': '/tmp'}); | ||
| fs.mkdirSync = jest.fn(() => {throw 'Could not create artifacts root dir'}); |
There was a problem hiding this comment.
Throw a new Error(''Could not create artifacts root dir') so we can follow the stack trace
| @@ -30,6 +31,18 @@ class Detox { | |||
| this.userConfig = userConfig; | |||
| this.client = null; | |||
| this.device = null; | |||
| this._currentTestNumber = 0; | |||
| const artifactsLocation = argparse.getArgValue('artifacts-location'); | |||
| if(artifactsLocation !== undefined) { | |||
There was a problem hiding this comment.
why not use if(artifactsLocation) instead ?
There was a problem hiding this comment.
That's unsafe in JS in general due to implicit type conversions, so I think it's better to never use it.. Here for instance, if a user supplies an empty string, like --artifacts-location '', then JS will convert artifactsLocation to false.
| class ArtifactsPathsProvider { | ||
| constructor(destinationParent) { | ||
| if(!destinationParent) { | ||
| throw 'destinationParent should not be undefined'; |
There was a problem hiding this comment.
throw new Error(''). I think this error string is not descriptive enough.
detox/src/devices/Device.js
Outdated
|
|
||
| this.deviceDriver.validateDeviceConfig(deviceConfig); | ||
|
|
||
| this.relaunchApp = this.relaunchApp.bind(this); |
There was a problem hiding this comment.
Why do we need all those binds ?
There was a problem hiding this comment.
I probably don't need all of them, only those whose this is incorrect and is called from outside this. I guess I can reduce the list, but some will have to stay there
detox/package.json
Outdated
| @@ -81,6 +82,10 @@ | |||
| "functions": 100, | |||
| "lines": 100 | |||
| } | |||
| } | |||
| }, | |||
There was a problem hiding this comment.
Not sure excluding src/index.js is the right thing. This is something we can test, it's actually important that we do, till we have a sane type system, we want to make sure beforeEach and afterEach exist and that they don't crash the test if detox is undefined.
There was a problem hiding this comment.
Oops, committed this by mistake, I actually added the the test for them, but forgot to remove the exclusion, will revert it.
detox/src/devices/Device.js
Outdated
| @@ -23,7 +34,52 @@ class Device { | |||
| await this.relaunchApp({delete: !argparse.getArgValue('reuse')}); | |||
| } | |||
|
|
|||
| async _copyArtifacts() { | |||
There was a problem hiding this comment.
I think this is not the job of Device to move artifacts around. Why not merge all the logic in an ArtifactsHelper as you previously suggested ?
No description provided.