note: we agreed to handle or at least re-consider this in the follow-up, so just a reminder here: having such default configPaths looks weird to me, but if it's fine for everyone else then we should at least mention this in README.md (same for runServers).

as a result of moving things to cli, this was resolved and config paths are no longer referenced here. take another look?

question: is there any reason why you don't use the latest available versions of deps? And IIRC we're trying to use "caret" versions whenever possible.

My reasoning was to use the same versions that are currently being used, without upgrading dependencies.

Why does it matter what versions are currently used in Kibana if we introduce isolated (once we get rid of FTR dependency), test-only and brand new package with its own dependencies and have a chance to not rely on something that's outdated (both dependencies themselves and the way we define them in package.json since we commit yarn.lock)?

Just to be clear, it's not a big deal at all and we can discuss it after this PR is merged, I'm just trying to find common ground on this matter and not to raise this question again :)

note: I believe we shouldn't process both "normal" arguments (configPaths) and argv in the same function, there should be a dedicated function/cli that parses command line arguments into string[] and passes it to runTests. Having said that, I'm totally fine if we discuss it in the follow-up. (same for runServers)

Take another look? I made some cli functions using only getopts. Commander would be overkill at the moment because we also need special args to be passed to functional_test_runner. I think this should be resolved in a followup.

optional nit: what if we:

• remove await in front of resolve since it's sync function
• mark configPath as const and not reuse it for both "resolved" and "non-resolved" path versions
• limit scope of try/catch only to piece of code we expect errors to come from

for (const configPath of configs) {
  try {
    await runWithConfig(resolve(KIBANA_ROOT, configPath), runEs, runKbn);
  } catch (err) {
    fatalErrorHandler(err);
  }
}

But since runWithConfig already wraps its content into the same try/catch we don't need to duplicate error handling in this function and it feels like we can just inline content of runWithConfig here.

Changes are arguable, hence optional :)

question: what if we use options: { runEs = runElasticseach, runKbn = runKibanaServer }? With this we'll be able to only override how we run Kibana if we want to. Or you think it's not a valid use case? (same for runServers)

That's a valid use case. Let's do that!

optional nit: hmm, maybe let's just move esTestConfig.getUrlParts() to a variable (it's called 4 times in this function)?

or rather, use the existing servers.elasticsearch definition in this file. ;)

question: this file isn't needed anymore, right (:tada:)?

Yes, I thought I deleted it!

Ah, it is deleted from x-pack but not from kibana.

question: should we start sentence for every list item from the capital letter?

Fixed-- made them all lowercase.

nit: no more x-pack-elasticsearch (and x-pack-kibana below), right?

Okay, replacing with X-Pack

issue: it seems there is no functional_tests_single.

This should be replaced with using kbn/es, and doing so the following files can be removed:

• packages/kbn-test/src/functional_tests/lib/find_most_recently_changed.js
• packages/kbn-test/src/functional_tests/lib/tarball.js
• x-pack/dev-tools/functional_tests/lib/tarball.js
• x-pack/dev-tools/functional_tests/lib/find_most_recently_changed.js

And some paths in packages/kbn-test/src/functional_tests/lib/paths.js and x-pack/dev-tools/functional_tests/lib/paths.js can also be removed

Added comments inline which should allow you to entirely get rid of runEsWithXpack.

This should be replaced with using kbn/es,

So, you're saying we get rid of runElasticsearch and runEsWithXpack, both?

^ Nevermind, yes, that makes sense. Both would go away, or just be simple methods that use kbn/es

Since running with or without x-pack is an option presented to kbn/es with license. We can probably get rid of this altogether and add as part of the configuration.

The SAML tests need their own config since they require specific ES/Kibana configuration, correct?

Yes, the idea was to move all that SAML-specific logic from the start/run methods and into config instead.

Changing this to an object would allow us to directly pass options to kbn/es installSnapshot/installSource.

Thinking something shaped like this:

{
    license:  'basic',
    password: 'changeme',
    esArgs: []
}

Great, that's a good plan. But then they have a different appearance than the kibanaServerArgs. I guess that doesn't look too bad.

Maybe the name should be something else, since esArgs: [] within is now the array.

@tylersmalley Your thoughts on the changes here? Did some work last Thursday to use kbn/es here, but still using proc.

(I named this myClusterStart just to try this out. Name needs to be better)

@tylersmalley How does this work today? Originally we had this

    fs.createReadStream(
      resolve(XPACK_KIBANA_ROOT, 'test/saml_api_integration/fixtures/idp_metadata.xml')
    ).pipe(
      fs.createWriteStream(resolve(esExtractPath, 'config', 'idp_metadata.xml'))
    );

to copy the idp_metadata.xml file into the config dir of the elasticsearch install path. How is the current code able to reference this path without a permissions error? Locally (with my PR), I see

java.security.AccessControlException: access denied ("java.io.FilePermission" 
"/Users/archana/Development/apps/elastic/versions/master/kibana/x-pack/
test/saml_api_integration/fixtures/idp_metadata.xml" "read")

You just need to pass it as an absolute path, kbn/es will ensure any absolute path to a file is copied to the config directory before running.

https://github.com/elastic/kibana/blob/master/x-pack/dev-tools/functional_tests/lib/run_es_with_xpack.js#L38

https://github.com/elastic/kibana/blob/master/packages/kbn-es/src/utils/extract_config_files.js