Add pluggable panel action tests

[stacey-gammon]

Adds a plugin nested in the test folder to be used for testing purposes.

Adds a flyout session object that is very similar to the inspector session object - need to sync with @timroes and maybe design on the best way to move forward with that. Should we allow multiple flyouts open at the same time? Or only one? And if only one (which is what I would lean towards) we should probably refactor out the inspector session to be a more generic flyout session so pluggable panel actions can easily mount a flyout without it having to specifically be an inspector flyout.

flyout's should also be allowed to be modal (like dashboard's add panel).

Similarly I'd like to expose an easy way for panel actions to display a pop up modal, which probably needs a similar session object to ensure there is only one open at any given time.

[stacey-gammon]

@elastic/kibana-platform - Ideally this infrastructure for the panel actions test would be generalized, so all plugins would have a place to add functional tests (rather than each plugin having it's own folder under test which won't be scalable). If I go down that path I have a couple questions:

• Are there any plans to deprecate the --plugin-path variable?
• Can I list multiple folders for --plugin-path? If so, would it be a comma separated list, or would I pass in multiple --plugin-path flags?

[epixa]

I do agree that tests within plugins is ideal, and we should work toward that. I'm not aware of there being any good solution for that currently that plays nicely with all of our test tooling and whatnot.

We have to remove plugin-path. If people cannot rely on the location of plugins, then we can not reliably support sharing code statically with and between plugins. We have webpack aliases in the browser that allow us to circumvent this problem, but that's only true for as long as we ship webpack with the builds, and that won't be the case sooner rather than later. We don't have any solution for this on the server, so even today our browser/server imports behave differently.

Even using link: dependencies in plugin-specific package.json files (my preference for static code sharing in the long run) requires consistent static path references.

I haven't given this too much thought, but one solution might be shipping mock plugins with the tests in a plugin and then having some mechanism of registering those plugins with the test tooling. Then when we run tests, the tooling could symlink in the relevant plugins to the kibana-extra directory, which is where they will actually be used in Kibana.

Ultimately though, I think we should consider testing these plugin boundaries in a totally different way. I think running all of Kibana along with a special plugin just to test a plugin contract is a little crazy. That's basically how we treat all of our functional testing today, but it doesn't have to be that way. The new platform brings more clearly defined boundaries between plugins, core, and even individual services. We should be able to perform functional testing of any given plugin contract programmatically by wiring up the contracts for things it depends on, invoking the lifecycle functions that are relevant for the test, and then verifying the behaviors/side effects of that plugin's contract.

[stacey-gammon]

@epixa - I agree that all sounds great for the mid-long term, but can I use --plugin-path for the short term so at least I can have tests? Then we can just migrate the tests using whatever final mechanism the platform team comes up with. As I understand it there is no other way to add tests for these plugin points in the current system?

[timroes]

I would also like to see this still as a short term solution (maybe a bit more generalized, but already talked with Stacey that I could generalize that for more plugins, since visualize team has the very same needs). Also until every code lives in the new platform, it will still be months (or years?), so I think not having functional tests on our plugin APIs is not a solution we should go with. We've already had several regression bugs introduced, that weren't caught by unit tests, since you only would have actually got them when booting up the whole Kibana application and causing some side effects in there.

I really don't want to wait until we've all code moved to the new platform before I can get reliable tests on that, and before that there could always be sideeffects from the old platform model, that are causing our issues.

[epixa]

@timroes We don't have to wait for the new platform to solve this problem. The principles that the new platform was designed for which make functional testing systems without running complete Kibana installs aren't unique to the new platform, and we often are already applying them today.

@stacey-gammon What specific things do you want to test here? That new panel actions can be added? That added panel actions render in the dom? If we need to keep the plugin-path around for the sake of testing, we will, even if only for testing.

[timroes]

@epixa That's nice to hear that it would also work without the new platform. Will have to check how that looks in the end. But I think sometimes I want all of Kibana fired up for some types of tests, because we had a couple of issues, that especially only occurred because all of Kibana was there and some side effects from other part of code were causing an issue.

Let me also give an answer to the question you asked Stacey, for what I would like to have this tests available (and why I will try to generalize that PR as soon as it passed tests, into something that can run on multiple plugins).

The visualization team would require a solution like this to test mainly for the APIs like the visualize loader, that they work fine when using them in a plugin. We want to test if calling the API really leads to the correct rendered result in the browser (so something that we can't cover by unit tests). Also I want to test for some combinations on how to call the API that are available, but we don't use them inside of Kibana that way (e.g. calling the loader for a chart, on a non time-based index, and don't pass a timeRange in there - in Kibana we always pass the time range in there). So that's were these tests would become very handy (and looking at the past issues, we would have been able to most likely prevent 2 blocker issues in the most recent release by having that tests available).

The next part why I think that will be very useful is documentation. Since we currently don't have a proper developer documentation system (and even if we would) it's a very good way to write documentation for your API: generate a well documented plugin that shows all the different uses, and link to that in appropriate places. This approach has two major benefits over just documentation (even in case of a proper developer documentation system):

1. Your "documentation" aka sample plugin is actually automated tested. That makes it nearly impossible to - and we've had that a couple of times in the past - forget updating documentation when some minor parameters changed. Since we actually run tests against the plugin, we need to update it, and automatically have an always up-to-date sample plugin in place.

2. You more easily test the whole development experience, and not just your API, e.g. is your plugin's index.js correctly, are there any changes that would actually require updating those. E.g. we need a specific set of uiExport imports for the vis loader to run. Some change actually can (and did) require a new uiExport to need to be imported. That change even doesn't need to be inside the visualize loader code itself, but maybe one visualization type requires now a different uiExport to function properly. This is something that will be very easily overlooked when writing docs. It's something that is still very likely not to be caught by any unit tests, but only when doing a full functional use test of that plugin.

[epixa]

I have a lot of thoughts on this, but I don't want to block rolling out better test coverage on a higher level conversation about testing. Go ahead and use plugin-path, and when we are able to get rid of it from the product, we can in the worst case scenario just enable it for testing.

One thing to keep in mind though: the work has begun to remove the optimizer, so any plugins that you want to run in a non-dev version of Kibana need to be compiled, which is not something we can reliably do in advance until the optimizer is actually removed. Given that our current functional tests can be/are run on real builds, you should pull plugin tests out into a new jenkins job so we can make sure Kibana is running in dev mode.

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request

[azasypkin]

note: does this really give us any benefit comparing to just ts-ignore like we do for other cases?

We talked about that inline declare module thing with @timroes the other day and obviously the ideal option would be to have types file in "eui" package, even very simple and incomplete types would be better than that, but only if eui owners would commit to keep these typings up to date. There is no need to write them all, just keep eye on existing types during review. Is it something we can do? Who is the owner of this package? @cjcenizal ?

[stacey-gammon]

Soon as we have a new eui package I can get rid of this. I went into eui and added the typings (elastic/eui#1001).

I think @chandlerprall is taking charge of the typescript efforts in EUI. I'm not sure what the plan is, but it seems a lot easier to upkeep if EUI used typescript instead of maintaining separate typings definitions. Perhaps that is the long term plan.

I'll wait for a new eui to submit this PR so I can get rid of this.

[cjcenizal]

Yep, Chandler owns the code side of EUI now. I agree that converting all of EUI to TS would make the most sense for ease-of-maintenance.

[chandlerprall]

Converting EUI to TS is the most likely solution. Either way we know we need to properly support TS consumers.

[stacey-gammon]

I'll wait for a new eui to submit this PR so I can get rid of this.

I well underestimated the time it'd take to be able to take advantage of some new code in EUI so I take this back. I'd like to not block this on the eui bump which has been hitting it's own test failures (whether flaky or legit, undetermined).

If I can get the ci passing today I'd love to be able to merge before I'm out on vacation starting tomorrow.

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request

[stacey-gammon]

Needed to update jest snapshots for inspector panel. Manual inspector looks good to me:

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request

[stacey-gammon]

@chrisdavies this one is ready to go if you don't mind giving a review

[chrisdavies]

Typo: An -> A

[chrisdavies]

LGTM.

Made some minor suggestions, mostly comment typos, and some incorrect license comments (I think), and a possible alternative that might make using flyouts a bit more idomatic.

I also pulled it down and ran it, and it all seems to work fine.

[chrisdavies]

Typo: And -> An (or The)

[chrisdavies]

Typo: stil -> still

[chrisdavies]

I think this may be incorrect. I think this is OSS.

[chrisdavies]

Same here, no?

[chrisdavies]

I have a few thoughts about this. Should EuiFlyout itself just coordinate this internally, essentially acting like a singleton and ensuring only one flyout is ever visible at a time? It almost seems like that would be a sensible design.

Alternatively, we could do that ourselves, and keep this inside the React lifecycle by just making it a plain React component. Something like this, maybe:

// Something like this
let currentFlyout = null;

class SingletonFlyout {
  componentWillMount() {
    if (currentFlyout) {
      currentFlyout.close();
    }
    currentFlyout = {
      flyout: this,
      close: this.props.onClose,
    };
  }

  componentWillUnmount() {
    if (currentFlyout && currentFlyout.flyout === this) {
      currentFlyout = null;
    }
  }

  render() {
    return (
      <EuiFlyout>
        {this.props.children}
      <EuiFlyout>
    );
  }
}

The advantage there is that consumers can use it like any other React component. It'd be styled to have a fixed position, etc, but could show/ hide based on props / state changes without special cases or whatever.

It'd be consumed like this:

// in some stateful component's render...
render() {
  return (
    <div>
      My usual component.
      <button onClick={() => this.setState({ isFlying: true })}>
        Fly, little bird!
      </button>
      {!this.state.isFlying ? null :
        <SingletonFlyout onClose={() => this.setState({ isFlying: false })}>
          <h1>Hi!</h1>
        </SingletonFlyout>
      }
    </div>
  );
}

Or something.

You could pass it an Angular scope as an optional param, too, if that's a case that needs handling.

The advantage to either of those ^^^ approaches is that the lifecycle of the flyout is normal React stuff, no special DOM manipulation or possible leaking edge cases.

If we keep the current solution (which is fine by me), it seems like it might be nice to optionally pass the container in, so that callers could pass a container that is managed by React / destroyed by React and not have to worry about cleanup or extraneous references.

[stacey-gammon]

I prefer your first solutions - having flyout be a singleton. I can't think of any good situation where we should have two flyouts.

I would prefer to leave it as is though in this PR, just for the sake of unblocking some things (Tim wants to add inspector tests, I want to extend the panel action tests, and we both want to generalize this approach to all plugins not just panels). I think we'll have to address the flyout changes soon, though, as what should happen if you open a panel action with a flyout while an inspector is open?

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request