Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Discuss] Functional tests for NP client side #52768

Closed
wants to merge 1 commit into from

Conversation

mshustov
Copy link
Contributor

@mshustov mshustov commented Dec 11, 2019

Summary

I want to discuss with @elastic/kibana-platform how to write integration tests for the non-UI client logic.
We have karma tests currently but it seems that we are deprecating them without providing an alternative solution.
I tried to reuse existing functional tests infrastructure because:

  • we run the full Kibana & ES setup.
  • we have control over elasticsearch. what is important for us: data preload, license type, roles, etc.

My pain points with the current setup:

  • We cannot have access to the platform in runtime. It means we cannot test lifecycles properly.
  • We cannot subscribe to any events in js runtime.
  • We cannot have access to all the plugins. I had to use the Legacy platform plugin to work around this. Initially, I started all that effort to add some proper tests for the Licensing plugin.
  • We are quite limited in executing js in the browser context. What we can do here?
    • To build the framework to control server-side on the client (ES mostly).
    • To put up with the current limitations and add runtime (global ?) utils to facilitate testing in the browser.

What other alternatives we can consider?
I believe
https://github.com/puppeteer/puppeteer, https://www.cypress.io/
have the same limitations regarding js-runtime. Although Cypress has nice support for assertions
cypress-io/cypress-documentation#1109

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

@mshustov mshustov added discuss Team:Operations Team label for Operations Team Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc labels Dec 11, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform (Team:Platform)

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-operations (Team:Operations)

expect(hasAccessToInjectedMetadata).to.equal(true);
expect(
await browser.execute(() => {
return window.np.setup.core.injectedMetadata.getKibanaBuildNumber();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

won't work on a NP app page, because we don't run LP

let value;
window.np.setup.core.uiSettings.get$('ui_settings_plugin').subscribe(v => (value = v));
callback(value);
return Promise.resolve(value);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is here just to mute TS error. I didn't manage to make it work with a promise. @dmlemeshko FYI

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the node-browser communications are, per nature, limited. To my knowledge browser.executeAsync will always be using callbacks

Copy link
Contributor Author

@mshustov mshustov Dec 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like there is a wrong type on the our side
https://github.com/elastic/kibana/blob/master/test/functional/services/browser.ts#L474
Because in Selenium types:

   * Unlike executing synchronous JavaScript with {@link #executeScript},
   * scripts executed with this function must explicitly signal they are
   * finished by invoking the provided callback. This callback will always be
   * injected into the executed function as the last argument, and thus may be
   * referenced with {@code arguments[arguments.length - 1]}. The following
   * steps will be taken for resolving this functions return value against the
   * first argument to the script's callback function:

from https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/selenium-webdriver/index.d.ts#L2316

@mshustov mshustov requested review from a team December 11, 2019 17:09

const settingsValueViaObservables = await browser.executeAsync((callback: Function) => {
let value;
window.np.setup.core.uiSettings.get$('ui_settings_plugin').subscribe(v => (value = v));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we cannot rxjs in the test file

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@joshdover
Copy link
Contributor

One alternative we may be able to create: build very basic karma-like functionality into the functional test runner. It could work something like this:

  • Write tests in a plain mocha test file that test the APIs exposed on window.np similar to how you do here.
  • Compile test file with babel, and load into browser with script tag
  • Run mocha suite and record results on some window property
  • Read results with Selenium

In theory we could write a functional test runner service that encapsulates all of this functionality and exposes it to functional tests.

@mshustov
Copy link
Contributor Author

I want to discuss the high-level architecture before coming to implementation.

Write tests in a plain mocha test file that test the APIs exposed on window.np similar to how you do here.
Compile test file with babel, and load into browser with script tag
Run mocha suite and record results on some window property

That would work as long as we set up all required Kibana & Elasticsearch states in the FTR and run this 'test bundle'. With this, each setup will require a separate 'test bundle'.

@joshdover
Copy link
Contributor

That would work as long as we set up all required Kibana & Elasticsearch states in the FTR and run this 'test bundle'. With this, each setup will require a separate 'test bundle'.

By 'setup' do you mean each FTR suite (each config file) or each test file? I think we could do this in a way that can be done on the fly each time this service gets invoked.

Copy link
Contributor

@pgayvallet pgayvallet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, this is a big one.

in-browser testing for non-functional behaviour has always been a pain. To be honest I'm not even sure this is something that should be achieved.

I will start by asking the big question: Now that JS testing environnement properly achieves to emulates a browser environment, with current transpilers 'ensuring' compatibility with major browsers, and with the postulate that we still have proper in-browser functionnal test suites for actual functional application behaviours, what are the actual upside to test non-UI behaviour in real browser environment instead of running them in jest with jsdom?

I tried to reuse existing functional tests infrastructure because:

  • we run the full Kibana & ES setup.
  • we have control over elasticsearch. what is important for us: data preload, license type, roles, etc.

I know the terms differ from a project to another, and from a company to another, but imho there is a subtle difference between integration tests and end-to-end/functional tests. These needs seems to be for the latest, which I'm not sure makes a lot of sense regarding core testing?

Basically I think that anything that could not be human-tested is not an end to end or functional test, but an integration test. And integration tests should probably not be running in browser environment.

We cannot have access to the platform in runtime. It means we cannot test lifecycles properly.

These should be covered in unit/integ testing, not functional imho.

We cannot subscribe to any events in js runtime.

This is a pain. That can be achieved with proper tooling and browser-side injected code (either via 'test' plugin, bootstrap code or browser.executescript, but the boilerplate to have that kind of things working is heavy (and ugly most of the time)

I started all that effort to add some proper tests for the Licensing plugin.

Yes, licensing is exactly the kind of 'not at the end of the pipe' component that is incredibly hard to test in real-time browser environ, as if what the ONLY plugin enabled on the platform, an user/human test could not test anything.

What other alternatives we can consider?

All the limitations you listed will be present in any browser e2e testing framework. However imho, if today we have the opportunity to choose, cypress and cypress tooling is amazing.

let value;
window.np.setup.core.uiSettings.get$('ui_settings_plugin').subscribe(v => (value = v));
callback(value);
return Promise.resolve(value);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the node-browser communications are, per nature, limited. To my knowledge browser.executeAsync will always be using callbacks

Comment on lines +22 to +25
window.np = {
setup: npSetup,
start: npStart,
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if this will be sufficient or if we will need to find a way to expose some more internal core APIs for our tests.

@mshustov
Copy link
Contributor Author

By 'setup' do you mean each FTR suite (each config file) or each test file? I think we could do this in a way that can be done on the fly each time this service gets invoked.

An example from functional tests:
https://github.com/elastic/kibana/pull/53002/files#diff-07f859f876c89b21dc1501a4ad80c574R79-R80
I setup elasticsearch within test runner and run test suite https://github.com/elastic/kibana/pull/53002/files#diff-07f859f876c89b21dc1501a4ad80c574R83

These needs seems to be for the latest, which I'm not sure makes a lot of sense regarding core testing?

Some core API depends on the elasticsearch. From this point of view:

  • we use unit-test to test observed behavior of the core API, input & output permutations, edge-cases. Not required browser env.
  • we use the integration test to cover the main use case for the whole stack. In this case, we may have to set up data preload, license type, roles, etc. Can be done with test_utils tools & api_integration functional tests. Not required browser env.
  • I'd prefer to have a least one functional smoke test for the browser counterpart of stack dependent API (uiSettings, SavedObjects, Licensing plugin) to cover a user story. Might require testing in the browser if we want to test the code in the same env as we ship it in prod.

With this classification, I'd say all the tests from the current PR should be integration tests (except an expired license banner). I'd be happy to move them, but there are several problems:

@pgayvallet
Copy link
Contributor

After writing a few FT, I think the most blocking point is (at least for me)

We cannot have access to all the plugins. I had to use the Legacy platform plugin to work around this. Initially, I started all that effort to add some proper tests for the Licensing plugin.

For #50223 and now #54366 I have to write navigation test between apps. Some behaviours can only be achieved by navigating from a NP app to another NP app. Which mean that in that case, I can't use the legacy core_provider_plugin to access core API via window.__coreProvider, as legacy plugins are not loaded on non-legacy mode.

I'm forced to inject manually my NP plugin start/setup API to another window.__Prop to be able to access and interact with it in my tests. It's some boilerplace that can quickly increase when we'll add more test plugins and functional tests, as every test case would need to rely on doing the same thing manually.

I think that from a functional test perspective, having a way to do what @restrry has done in test/plugin_functional/plugins/core_provider_plugin but handled directly from core could make sense.

We could for exemple have an environment variable (or another way to enable) to have core expose it whole API to a property under the window object to be able to access it from FT when in test mode/env, as the core_provider_plugin currently do.

WDYT?

@mshustov
Copy link
Contributor Author

We could for exemple have an environment variable (or another way to enable) to have core expose it whole API to a property under the window object to be able to access it from FT when in test mode/env, as the core_provider_plugin currently do.

I didn't go this way because I had seen no other test specific env flags. Introducing one, we add another small difference of the test env from prod env. OTOH I don't see any other less hack-ish ways, because isolation and explicit dependency declaration are the platform principles.

@pgayvallet
Copy link
Contributor

I didn't go this way because I had seen no other test specific env flags. Introducing one, we add another small difference of the test env from prod env. OTOH I don't see any other less hack-ish ways, because isolation and explicit dependency declaration are the platform principles.

If we want to avoid the 'test-only' flag, we could decide to actually add this flag as a 'public' setting/flag, that would ask core to expose it's inner and plugin contracts to a given property of the root window object.

Not sure if preferable though

@mshustov mshustov closed this Oct 17, 2020
@mshustov mshustov deleted the np-functional-tests-client branch October 17, 2020 08:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Team:Operations Team label for Operations Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants