CLI: Add pass through test runner arguments

[EdwardDrapkin]

It's helpful to use --bail sometimes in either Jest or Mocha, so I exposed it and forwarded it in.

[rotemmiz]

I think this can be passed using -- --bail to both test runners

[noomorph]

@rotemmiz , no, we have no -- passthrough logic in CLI yet.

[rotemmiz]

Then this is probably what we need to add instead of exposing flags individually.
This is a very limited solution.

[rotemmiz]

I would rather add a mechanism to pass args to test runners, kinda like npm does.
https://github.com/npm/npm/pull/5518/files#diff-c8ef191c95255343393f2a3256d29483R127

[EdwardDrapkin]

I updated this PR to allow generic pass through test runner arguments via an environment variable, DETOX_TEST_RUNNER_FLAGS.

I ran into two issues with the -- <extra args> approach: launching detox via yarn/npm (yarn detox test ...) swallowed the -- before commander could parse it, and commander itself swallows the -- before launching a subcommand like detox-test.js. So the approach of detox test ... -- --bail --forceExit seemed out, and passing the extra arguments as a flag seemed like a natural next idea. detox test ... --extraArgs='--bail --forceExit' seemed really awkward and prone to breakage with shell escape sequences and whatnot, so I settled on an environment variable, DETOX_TEST_RUNNER_FLAGS.

DETOX_TEST_RUNNER_FLAGS='--bail --forceExit' yarn detox test ... isn't the worst thing in the world, and using an environment variable means that people (like myself) can just set that environment variable in CI and get the bailout behavior across all tests without having to modify every invocation of detox.

The actual mechanism is pretty easy to change if you guys have another method you'd prefer, but I gave up on the -- mechanism and I'm not sure it's possible without changing commander upstream.

[EdwardDrapkin]

@rotemmiz is this approach acceptable or do you have an alternative you'd prefer?

[rotemmiz]

I'm against setting an env var to cli if is not absolutely needed, it makes the usage cumbersome and error prone in my opinion.
If it's not possible to add via -- , let's use --args instead.

[EdwardDrapkin]

I can't use args because program.args is reserved by commander and it doesn't seem that it will parse --args="--bail --forceExit" correctly. Was trying with this short script:

const program = require('commander');

program
    .allowUnknownOption()
    .option('-f [args]', 'foo')
    .parse(process.argv);

console.log(program);

It seems that the best I could do with args is --runner-args=bail,forceExit which means it's not possible to pass parameters to runner flags, or --runner-args="bail forceExit watchIgnorePattern='123'" which would allow them to be passed, but either approach seems much cludgier and error prone than the env var. It's looking more and more like the only solution that will work with commander's parser is the env var :S

[rotemmiz]

Do you believe that this can be handled properly by argparse?
I have been intending to switch for quite a while as commander has very critical and unhandled parsing errors.

[EdwardDrapkin]

The original python argparse say that it can do exactly that: https://docs.python.org/dev/library/argparse.html#partial-parsing

I will do a test Friday morning since tomorrow (Thursday) is a holiday and I won't be at a computer, but it looks like we should be able to just pass all un-detox flags directly into jest, so detox test ... --bail --forceExit would work fine, no need for -- or --args or an env var, which might be the best option. I'll test to see if I -- works or --args parses correctly, but even if they do, IMO just passing through all args is the best developer experience. And jest/mocha will error themselves out if there are nonsense flags passed to them, so the entirety of the args does eventually get validated. Are you okay with omitting the -- or --args if possible? That might also be possible with commander, albeit with some more work.

[noomorph]

@rotemmiz, @EdwardDrapkin , guys, I struggle a bit to see what's the problem here:

function splitPassthrough(argv) {
  const position = argv.indexOf('--');

  if (position === -1) {
    return [argv, []];
  }

  return [argv.slice(0, position), argv.slice(position + 1)];
}


const [argv, pargv] = splitPassthrough(process.argv);

console.log('argv', argv); // use with commander
console.log('pargv', pargv); // pass as-is

Why can't we make a helper to split argv and use with commander in a way it used to be and add a routine to append -- ... passthrough args to the final command?

[EdwardDrapkin]

Commander rewrites process.argv for subcommand invocations.

Although I guess we could pass [...process.argv] instead of process.argv

EDIT: nevermind, sigh, commander actually spawns a new process with an args struct it builds for subcommands, so I don't think we can parse process.argv directly from detox-test.js... there are a couple of things I can try though. If nothing else, I might be able to get the unknown flags parsed in the entry script and pass them as an environment variable if commander doesn't comply.

[EdwardDrapkin]

I spent some time digging in commander and it turns out it does parse the unknown args out and just drops them on the floor, but I was able to call the parser methods myself. It appears to work that extra flags just get passed clean through to the test runner.

[rotemmiz]

This is great stuff!! Thanks for the stubburn investigation!
Seems like this branch needs a rebase/merge from master? Can you please do it? I want to make sure CI is green before it gets merged.

[EdwardDrapkin]

There wasn't anything for me to rebase, so I pushed a documentation update to re-kick CI. Clicking through to the CI logs, it looks like some animations timed out in iOS 56, and I don't think anything I changed could have affected that - and if it did, not sure how it only affected one entry in the build matrix. I think it was just a random failure that periodically happens with simulators...

[rotemmiz]

Thanks a lot for that!

[noomorph]

Edward, thank you very much!