Switch CLI to yargs #1109

DanielMSchmidt · 2019-01-06T15:47:07Z

Switch commander interface out with yargs, which is a more reliable interface and the following awesome features:

We can abstract common patterns (which config do I have, which platform am I supposed to run, etc) into nice middlewares, making the actual CLI code more human readable (To be done, if we agree that it makes sense)
We can test each CLI command, which is really nice to be sure we don't break anything while improving the user experience
It supports CLI plugins out of the box (which I think is cool because it allows third parties to make the CLI more powerful without writing their own wrappers with shell outs
Yargs allows bash completion, I will investigate how this works, it was not my top prio
Extendable configs from files and package.json like with babel presets.
We can write a lot of validation inside the yargs real already, making this outer bound of our system as rigorous as possible, so we don't have to check everything 100 times within the main code base.

Resolves #1132.
Addresses #1238.

DanielMSchmidt · 2019-01-13T19:15:21Z

@rotemmiz This PR is done now. It enables users to use the detox cli via environment variables, over the package.json and via configuration file.

The CI seems to be flaky, so 🤷‍♂️

rotemmiz · 2019-01-22T20:44:00Z

detox/local-cli/build-framework-cache.test.js

@@ -0,0 +1,26 @@
+describe('build-framework-cache', () => {


I believe these kind of tests have not a lot of meaning, it just reimplements the production code, and checks that they are the same. I have a feeling we can give build-framework-cache.test.js completely up.

rotemmiz · 2019-01-22T20:44:53Z

detox/local-cli/build.test.js

@@ -0,0 +1,115 @@
+describe('build', () => {
+  it('shows help text', async () => {


We should probably drop shows help text test, what does it check?

rotemmiz · 2019-01-22T20:46:38Z

detox/local-cli/build.test.js

+    mockPackageJson({
+      configurations: {
+        only: {
+          build: 'echo "I was build"'


Just for the sake of readability, can you change the strings to corespond to the config names?

rotemmiz · 2019-01-22T20:48:05Z

detox/local-cli/build.test.js

+        }
+      }
+    });
+    const mockExec = jest.fn();


Can

const mockExec= jest.fn(); jest.mock('child_process', () => ({ execSync: mockExec }));

be moved to the top of the suite? you add it in beforeEach

rotemmiz · 2019-01-22T22:08:42Z

detox/local-cli/clean-framework-cache.test.js

+  it('shows help text', async () => {
+    jest.spyOn(process, 'exit'); // otherwise tests are aborted
+
+    expect(await callCli('./clean-framework-cache', '--help')).toMatchInlineSnapshot(`


same here, meaningless test

rotemmiz · 2019-01-22T22:22:03Z

detox/local-cli/test.js

+  'artifacts-location': {
+    alias: ['a', 'artifactsLocation'],
+    group: 'Debugging',
+    describe: '[EXPERIMENTAL] Artifacts (logs, screenshots, etc) root directory.',


Actually it is not experimental anymore, if you can drop the [EXPERIMENTAL] text across the artifacts args, it would be super

rotemmiz · 2019-01-22T22:25:26Z

detox/local-cli/test.js

+
+  clearDeviceRegistryLockFile();
+
+  if (!program.configuration) {


There are many validation/assigning steps here, does yargs have a custom processing functionality for args? if it does, it should probably go in there.

It does through middlewares, but I would suggest to move it into a separate PR :)

rotemmiz · 2019-01-22T22:26:57Z

detox/local-cli/test.js

+  }
+
+  function runMocha() {
+    const loglevel = program.loglevel ? `--loglevel ${program.loglevel}` : '';


can we do something automatic with this section? I hate adding a new param every time its being added to cli api.

🤷‍♂️ Not that I'm aware of, maybe we should have a map between our flags and mocha or jest flags.

Interesting...

I still hate the fact that mocha has no mechanism like jest for having environment. This would be way more natural from a jest perspective than the wrapping that we currently do

detox/local-cli/test.test.js

rotemmiz · 2019-01-22T22:29:22Z

docs/APIRef.DetoxCLI.md

@@ -34,6 +34,53 @@ detox [options] [command]
 | --- | --- |


DanielMSchmidt · 2019-01-25T14:53:56Z

Also to be done: #1083 (comment)

Redo #1083

DanielMSchmidt · 2019-01-29T22:00:44Z

@rotemmiz This is ready for review now

rotemmiz

Thank you Mr Schmidt!
There are a few minor issues regarding cleanup and location of new files, please see my notes.

detox/setupJest.js

docs/APIRef.DetoxCLI.md

detox/package.json

rotemmiz · 2019-01-30T22:11:47Z

detox/cli.js

@@ -0,0 +1,23 @@
+#!/usr/bin/env node


Why did you prefer putting this here and not inside /local-cli ?

I can not put . as commandDir and I did not want to have this extra level of nesting 🤷‍♂️

Why not?

- "detox": "local-cli/detox.js" + "detox": "cli.js"

This is a part of the changes you made in this PR, why can't it be reverted?

Because of this part in my changes. At least as far as I understand it. Feel free to try it, though.

rotemmiz · 2019-01-30T22:12:26Z

detox/local-cli/init.js

+  console.log("init handler", argv)
+  switch (argv.runner) {
+    case "mocha":
+    createMochaFolderE2E();


please indent the code

rotemmiz · 2019-01-30T22:12:53Z

detox/local-cli/test.js

+
+  clearDeviceRegistryLockFile();
+
+  if (!program.configuration) {


DanielMSchmidt · 2019-02-03T09:54:52Z

@rotemmiz Answered and fixed everything 💪

rotemmiz · 2019-02-07T20:19:49Z

@DanielMSchmidt I moved cli.js to local-cli.js.
I am still having a weird issue I can not trace:
running detox test --configuration ios.sim.release results in

node_modules/.bin/mocha e2e --opts e2e/mocha.opts --configuration ios.sim.release  --no-colors    --grep :android: --invert  --record-logs none --take-screenshots none --record-videos none --artifacts-location "artifacts/ios.sim.release.2019-02-07 20-17-13Z"  --testRunner mocha --configurations [object Object]

I couldn't find where --configurations is being added to program. The only place it exists in is:

const config = require(path.join(process.cwd(), 'package.json')).detox;

But I can't see how that is applied into program.
Your help is appreciated here :)

DanielMSchmidt · 2019-02-07T22:46:23Z

@rotemmiz I think the problem is that we don't successfully filter out the options passed via CLI, they just get passed through as it seems

rotemmiz · 2019-02-07T23:05:38Z

The issue seems bigger than the purposed fix in a0582e3.
Currently --testRunner is also leaked into collectExtraArgs.
Seems like these are all parameter of the detox config from package.json. I couldn't find where this is injected, but it probably should be filtered from collectExtraArgs as well.

DanielMSchmidt · 2019-02-07T23:14:35Z

@rotemmiz Ah yeah, that explains why I did not catch it in the very first moment, when this feature is not enabled, interesting 🤔 I'll think a bit about how to fix this, I mean we basically would also need to be carful about all other input options, so just filtering them out might not be the best option

rotemmiz · 2019-02-07T23:16:26Z

Both confiurations and test-runner/testRunner are not CLI args, they are configuration parameters, they are somehow leaked into program.

DanielMSchmidt · 2019-02-10T07:49:29Z

@rotemmiz I think I got it, at least theoretically. We could create a new instance of a yargs program without any configuration within collectExtraArgs, then it should work just fine.

…d cli code

DanielMSchmidt requested a review from rotemmiz as a code owner January 6, 2019 15:47

DanielMSchmidt force-pushed the danielmschmidt/move-to-yargs branch 2 times, most recently from cbe9916 to 3fcd99c Compare January 13, 2019 18:01

rotemmiz requested changes Jan 22, 2019

View reviewed changes

rotemmiz mentioned this pull request Jan 25, 2019

deprecated --file and --specs #1133

Closed

2 tasks

Daniel Schmidt added 11 commits January 29, 2019 22:54

move from commander to yargs

aa6bbbd

enable tests for local cli

b3aac24

write tests for local-cli

3437723

refactor test helpers into jest setup

8327903

add tests to test command

99254f7

allow environment variables to set argument values

848b582

allow package.json to configure command line flags

c19dca9

allow file to configure flags

92a6f14

remove unnecessary tests

d8e9d23

remove EXPERIMENTAL note

461a13c

refactor tests

9c401ca

DanielMSchmidt force-pushed the danielmschmidt/move-to-yargs branch from b7b726c to b189264 Compare January 29, 2019 21:54

quote test folder path

5ff5dfd

Redo #1083

DanielMSchmidt force-pushed the danielmschmidt/move-to-yargs branch from b189264 to 5ff5dfd Compare January 29, 2019 22:00

rotemmiz requested changes Jan 30, 2019

View reviewed changes

Daniel Schmidt added 4 commits February 3, 2019 10:43

document order of precedent for CLI

d1fb062

move setup jest to test dir

edb2f13

remove commander

9fd7189

indent code

27350bb

moved cli.js to local-cli dir + minor text changes

5a28f11

blacklist configurations

a0582e3

noomorph added 7 commits March 14, 2019 14:17

Merge branch 'master' into danielmschmidt/move-to-yargs

accf54e

Merge branch 'master' into danielmschmidt/move-to-yargs

d51d4ec

test: update local-cli test

8836aee

Merge PR #1133 functionality

8d535e8

code: refine cli arg description

950b595

fix: bringing backward-compatibility

4228446

ci: ignore __tests__ coverage

cbe2a76

LeoNatan mentioned this pull request Mar 22, 2019

Detox Instruments Integration (Phase 1) #1165

Merged

10 tasks

finalized deprecations, updated docs, fixed Windows issues, simplifie…

8f5b654

…d cli code

noomorph mentioned this pull request Mar 22, 2019

feat: device.takeScreenshot(name) #904

Merged

4 tasks

noomorph added 8 commits March 22, 2019 16:56

fix: broken link on CI

e28441f

fix: documentation and scripts per migration guide

00d4d42

Merge branch 'master' into danielmschmidt/move-to-yargs

555ab79

code: fixed phrasing

715b672

docs: fix wording

f30774f

docs: fix formatting in migration guide

0bb6fbb

docs: markdown formatting fix

6f6483b

bump ci

9e00d46

noomorph mentioned this pull request Mar 26, 2019

pass in files to the cli like jest and mocha do by default #1132

Closed

bump ci

cba0a4c

noomorph mentioned this pull request Mar 27, 2019

Improve configurability of Detox #1238

Open

13 tasks

noomorph merged commit 9f4cb01 into master Mar 27, 2019

noomorph deleted the danielmschmidt/move-to-yargs branch March 27, 2019 08:10

lock bot locked as resolved and limited conversation to collaborators Mar 30, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch CLI to yargs #1109

Switch CLI to yargs #1109

DanielMSchmidt commented Jan 6, 2019 •

edited by noomorph

Loading

DanielMSchmidt commented Jan 13, 2019

rotemmiz Jan 22, 2019

rotemmiz Jan 22, 2019

rotemmiz Jan 22, 2019

rotemmiz Jan 22, 2019

rotemmiz Jan 22, 2019

rotemmiz Jan 22, 2019

rotemmiz Jan 22, 2019

DanielMSchmidt Jan 29, 2019

rotemmiz Jan 30, 2019

rotemmiz Jan 22, 2019

DanielMSchmidt Jan 29, 2019

rotemmiz Jan 29, 2019

DanielMSchmidt Jan 29, 2019

rotemmiz Jan 22, 2019

DanielMSchmidt commented Jan 25, 2019

DanielMSchmidt commented Jan 29, 2019

rotemmiz left a comment

rotemmiz Jan 30, 2019

DanielMSchmidt Feb 3, 2019

rotemmiz Feb 3, 2019

DanielMSchmidt Feb 3, 2019

rotemmiz Jan 30, 2019

rotemmiz Jan 30, 2019

DanielMSchmidt commented Feb 3, 2019

rotemmiz commented Feb 7, 2019

DanielMSchmidt commented Feb 7, 2019

rotemmiz commented Feb 7, 2019

DanielMSchmidt commented Feb 7, 2019

rotemmiz commented Feb 7, 2019

DanielMSchmidt commented Feb 10, 2019

		@@ -0,0 +1,115 @@
		describe('build', () => {
		it('shows help text', async () => {

Switch CLI to yargs #1109

Switch CLI to yargs #1109

Conversation

DanielMSchmidt commented Jan 6, 2019 • edited by noomorph Loading

DanielMSchmidt commented Jan 13, 2019

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

DanielMSchmidt commented Jan 25, 2019

DanielMSchmidt commented Jan 29, 2019

rotemmiz left a 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

DanielMSchmidt commented Feb 3, 2019

rotemmiz commented Feb 7, 2019

DanielMSchmidt commented Feb 7, 2019

rotemmiz commented Feb 7, 2019

DanielMSchmidt commented Feb 7, 2019

rotemmiz commented Feb 7, 2019

DanielMSchmidt commented Feb 10, 2019

DanielMSchmidt commented Jan 6, 2019 •

edited by noomorph

Loading