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

Add testing document [skip-ci] #53514

Merged
merged 3 commits into from
Jan 10, 2020

Conversation

joshdover
Copy link
Contributor

@joshdover joshdover commented Dec 18, 2019

Summary

Relates to #53135

Adds a new documentation file to cover testing recommendations.

TODO:

  • Answer TODO about how to write integration tests for UI code
  • Figure out how to test Observables correctly
  • Move to asciidocs?

Checklist

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

For maintainers

@joshdover joshdover force-pushed the np/test-documentation branch from 745dd04 to 68bebce Compare December 19, 2019 22:30
@joshdover joshdover added Feature:New Platform release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.6.0 labels Dec 19, 2019
@joshdover
Copy link
Contributor Author

This is not ready to merge quite yet, but I'm opening it for review / feedback on the general format.

I would like to get this merged even before all of the different sections are filled out so that it can be useful sooner than later. The other sections can be filled out in other PRs.

@joshdover joshdover marked this pull request as ready for review December 19, 2019 22:38
@joshdover joshdover requested a review from a team as a code owner December 19, 2019 22:38
@elasticmachine
Copy link
Contributor

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

In general, we recommend three tiers of tests:
- Unit tests: small, fast, exhaustive, make heavy use of mocks for external dependencies
- Integration tests: higher-level tests that verify API behavior through sending real HTTP requests to Kibana server
- **TODO: what's the equivalent of integration tests for frontend code? Karma replacement?**
Copy link
Contributor

Choose a reason for hiding this comment

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

Tests for a public API without a UI-part. For example, license$ in licensing plugin, session control in the security plugin, etc.

src/core/TESTING.md Outdated Show resolved Hide resolved
src/core/TESTING.md Outdated Show resolved Hide resolved
src/core/TESTING.md Outdated Show resolved Hide resolved

#### HTTP Routes

_How to test route handlers_
Copy link
Contributor

Choose a reason for hiding this comment

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

Plugins can test their route handlers via FTR or existing test_utils. We can describe both ways until #51150
Can be done in follow-ups

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep that is the plan, to do in follow up PRs


These long-lived sessions make cleanup more important than before. It's entirely possible a user has a single browsing session open for weeks at a time, without ever doing a full-page refresh. Common things that need to be cleaned up (and tested!) when your application is unmounted:
- Subscriptions and polling (eg. `uiSettings.get$()`)
- Open connections (eg. a Websocket)
Copy link
Contributor

Choose a reason for hiding this comment

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

global API calls as well. that a plugin doesn't redirect the app on a specific url on the mount, for example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Core API calls is a good example. For instance, calling core.chrome.setIsVisible() for a full-screen mode, but not unsetting it when unmounted if user is redirected to another app.

Could you explain this example more? "a plugin doesn't redirect the app on a specific url on the mount"

Copy link
Contributor

Choose a reason for hiding this comment

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

This example is not possible to test. Probably to document it is enough.

setup(core: CoreStart, plugins: Deps) {
  // this logic would affect the whole Kibana
  plugins.licensing$.subscribe(async license => {
    if(!license.iAvailable) {
      window.href = ..
    });
  });
};

src/core/TESTING.md Outdated Show resolved Hide resolved
// Unmount UI
ReactDOM.unmountComponentAtNode(element);
// Close any subscriptions
settings$.complete();
Copy link
Contributor

@mshustov mshustov Dec 24, 2019

Choose a reason for hiding this comment

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

Well. If a plugin doesn't subscribe manually, it needn't to unsubscribe. We must explain this in the manual.
Given your example.
settings$ is Observable. A plugin cannot complete it. It will be completed if the unsubscription is implemented in the useObservable hook correctly.
The better example could be if a plugin implements polling for a every new settings$ value and implements a subscription manually.

const renderApp = (params: AppMountParameters, core: CoreStart): AppUnmount => {
  const settings$ = core.uiSettings.get$<string>('mysetting1');
  const pollingSubscription = settings$.subscribe(async url => {
    const value = await core.http.fetch(url);
    // ..
  });

  return function cleanup() {
    pollingSubscription.unsubscribe();
  };
};
...
describe('renderApp', () => {
  it('mounts and unmounts UI', () => {
    const params = { element: document.createElement('div'), appBasePath: '/fake/base/path' };
    const core = coreMock.createStart();
    const settings$ = new Rx.Subject<any>();
    core.uiSettings.get$.mockReturnValue(settings$);

    const unmount = renderApp(params, core);
    // Verify some expected DOM element is rendered into the element
    expect(params.element.querySelector('.some-app-class')).not.toBeUndefined();
    // Verify that subscribed to settings$
    expect(settings$.observers.length).toBeGreaterThan(0);

    unmount();
    // Verify the element is empty after unmounting
    expect(params.element.innerHTML).toEqual('');
    // Verify that unsubscribed correctly
    expect(settings$.observers.length).toBe(0);
  });
});

We can extract some of this logic in a custom matcher to be able to write something like this:

expect(settings$).toHaveNoSubscriptions();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the input here, Subjects make this much easier / possible. I think we should encourage this pattern for more fine-grained results but rely on something like drool to make this foolproof.


## Plugin Integrations

_How to test against specific plugin APIs (eg. data plugin)_
Copy link
Contributor

Choose a reason for hiding this comment

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

imho it makes sense to draw the line between API tests and plugin contract tests. With the NP we should encourage plugins to interacts via interfaces instead of API. It means a significant shift in testing as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added some guidelines on what this section should detail in the future, let me know if that lines up with what you're thinking.

The Kibana Platform introduces new concepts that legacy plugins did not have concern themselves with. Namely:
- **Lifecycles**: plugins now have explicit lifecycle methods that must interop with Core APIs and other plugins.
- **Shared runtime**: plugins now all run in the same process at the same time. On the frontend, this is different behavior than the legacy plugins. Developers should take care not to break other plugins when interacting with their enviornment (Node.js or Browser).
- **Single page application**: Kibana's frontend is now a single-page application where all plugins are running, but only one application is mounted at a time. Plugins need to handle mounting and unmounting, cleanup, and avoid overriding global browser behaviors in this shared space.
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably we should introduce autotests for memory leak detection. just a handful for the most critical parts https://github.com/samccone/drool and friends

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using drool to verify app unmounting cleaning would be interesting for sure. It appears we could have a single mechanism that tests each application automatically.

Copy link
Contributor

@mshustov mshustov Dec 31, 2019

Choose a reason for hiding this comment

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

should I create an issue to investigate this option? it's not a critical problem right now, can be done later

@joshdover joshdover force-pushed the np/test-documentation branch from 68bebce to 3bb4c0c Compare December 30, 2019 21:31
@joshdover joshdover force-pushed the np/test-documentation branch from 3bb4c0c to 3987242 Compare December 30, 2019 21:33
@joshdover
Copy link
Contributor Author

Some updates pushed based on first round of feedback.


_How to test your plugin's exposed API_

Guidelines:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it shouldn't be hidden in Testing docs and deserves a separate file https://github.com/elastic/kibana/pull/53866/files

@flash1293
Copy link
Contributor

Currently there are some karma tests doing visual regression testing (e.g. src/legacy/core_plugins/vis_type_vega/public/__tests__/vega_visualization.js). In a jest/ftr world it's not 100% clear to me where they belong. Is it recommended to cover them with the percy setup or will there be something closer to the current karma runners? Just food for thought, a section about visual regression testing would probably be helpful in some cases.

Copy link
Contributor

@mshustov mshustov left a comment

Choose a reason for hiding this comment

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

Let's merge as-is. We can update the remaining options in follow-ups.

@joshdover joshdover merged commit 0951faa into elastic:master Jan 10, 2020
@joshdover joshdover deleted the np/test-documentation branch January 10, 2020 16:49
@joshdover joshdover added v8.0.0 and removed v7.6.0 labels Jan 10, 2020
@joshdover joshdover added the backport:skip This commit does not require backporting label Jan 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:New Platform release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants