elastic / kibana

Your window into the Elastic Stack
https://www.elastic.co/products/kibana
Other
19.58k stars 8.09k forks source link

Instrument Kibana UI elements for Code Coverage #37698

Open LeeDr opened 5 years ago

LeeDr commented 5 years ago

Describe the feature: Several ideas listed here from a brainstorming chat.

The initial idea started with the known method to highlight elements in the UI with a red border via css during automated testing. @dmlemeshko implemented this locally just to see it work. (see screenshot below)

The next idea was to move that highlighting method to our FTR function that finds elements. That way it would also highlight elements that the tests find to get the value from. The thought is that this indicates something like code coverage, but maybe we call it element coverage.

But how do we "record" that information in a way that we can look at it and see what element's are not being checked?

That question led to the idea to record all the elements being found by the FTR to a file. After the full suite of tests complete we could then take that file of elements and highlight them in Kibana to allow us to see what isn't tested. But things like dialogs and flyouts might make this difficult.

@spalger proposed if we had data-test-subjs on all elements with a click handler (and even had a lint rule to enforce that) we could determine what elements we and were not clicked during the functional tests (element coverage). Any data-test-subjs that aren't clicked are essentially missing tests. This train of thought is generally assuming that all UI elements should have at least 1 UI functional smoke test (could be debatable).

We discussed the need for data-test-subjs to be globally unique. Possibly also a rule to have data-test-subjs on elements with variable driven data (data we should read in a test). Charts, maps, etc would probably not get covered by this?

In this screenshot from a video @dmlemeshko captured, we see the dashboard navigation button in the nav bar highlighted in red, as well as three fields in the timepicker. image

cjcenizal commented 5 years ago

This train of thought is generally assuming that all UI elements should have at least 1 UI functional smoke test (could be debatable).

The ES-UI team relies on Jest component integration tests for many UX paths which have traditionally been tested with functional tests. So any tools which enforce a hard opinion like this will need to let us opt-out. CC @sebelga @silne30

We discussed the need for data-test-subjs to be globally unique.

Not sure if this pertains to your use cases, but in our case at least we've found that because we only load one "view" at a time (e.g. list view, detail view, form view), generally speaking data-test-subj selectors only need to be unique per view, not globally.

spalger commented 5 years ago

data-test-subj selectors only need to be unique per view, not globally

This is my opinion as well, I expect we will capture the data-test-subj path to make them unique so we can have reusable components that have non-unique test subjects, but as long as they have a unique parent it's fine.

cuff-links commented 5 years ago

IS there a mechanic from within React where we could use a variable to inject a prop into the data test subject? I know in React you can pass props into child components. Let's say we are creating a combobox that will be the second combo box on the screen. The way I have seen the code in some instances is that box combo boxes will have the same data test subject. Is what I proposed a possible solution? Is there something similar or better implemented?

spalger commented 5 years ago

IS there a mechanic from within React where we could use a variable to inject a prop into the data test subject?

For sure: <SomeComponent data-test-subj={`foo-${variable}`} />

Let's say we are creating a combobox that will be the second combo box on the screen. The way I have seen the code in some instances is that box combo boxes will have the same data test subject.

That doesn't necessarily have to be a problem, as sometimes it's very hard to specify unique data-test-subj's for each component in a view.

Imagine a complex view component that has a whole form in it which is imported from another plugin or something. Assuming the elements within that view have sufficiently unique data-test-subjs within the view, we could support multiple instances of this complex view at the same time using parent data-test-subjs:

<div data-test-subj="container1">
  <ComplexView />
</div>
<div data-test-subj="container2">
  <ComplexView />
</div>

If you wanted to select the data-test-subj="name" element in the second complex view you can't just ask for name, as there are two on the page, but you can pass container2 name (space separated values) to the testSubjects APIs and it will select the data-test-subj="name" element within the data-test-subj="container2" element.

cuff-links commented 5 years ago

I agree with that. That definitely seems like something that should be in a best practices document somewhere.

LeeDr commented 5 years ago

@spalger you've mentioned using the data-test-subj of the parent but I don't know that we have a data-test-subj at a high level on each page (but would be great if we did). For example, if we logged all the selectors used during functional tests, I don't think I could group them by the pages they're on just based on thier names. But if there were 1 consistent data-test-subj at the very outer level of each page then that might be possible.

LeeDr commented 5 years ago

Here's an example where there's no data-test-subj on the index selection field in Discover app and so we're using 2 css selectors;

    async selectIndexPattern(indexPattern) {
      await find.clickByCssSelector('.index-pattern-selection');
      await find.setValue('.ui-select-search', indexPattern + '\n');
      await PageObjects.header.waitUntilLoadingHasFinished();
    }

I don't see any "discover" data-test-subj parent

<discover-app class="app-container">
  <!-- Local nav. -->
  <kbn-top-nav name="discover" config="topNavMenu"><div class="kbnTopNav" ng-show="kbnTopNav.isVisible()" data-test-subj="top-nav" aria-hidden="false">
...
</discover-app>
sebelga commented 5 years ago

I don't know very well what is available for selecting and targetting test subjects in functional tests but after adding a lot of tests in our apps I realized that I had much more flexibility by not giving unique id to test subject and being declarative.

<ul data-test-subj="myMenu">
  <li data-test-subj="menu-item">action 1</li>
  <li data-test-subj="menu-item">action 2</li>
</ul>

<!-- Instead of complex unique name -->
<ul data-test-subj="myAppHeaderMyMenu">
  <li data-test-subj="myAppHeaderMyMenuAction1">action 1</li>
  <li data-test-subj="myAppHeaderMyMenuAction1">action 1</li>
</ul>

The uniqueness of a DOM node is defined by the parent(s). And by being declarative we can also count the number of elements.

const menuItems = component.find('menu-item');

// read items
expect(menuItems.length).toBe(2);
expect(menuItems.map(item => item.text()).toEqual(["action1", "action2"]);

// click item
menuItems.at(1).simulate('click');

For each of the Elasticsearch UI plugins, I've added a top-level data-test-subj for each app. And through the Testbed helper find() we can target data-test-subj with dot notation and the path makes it unique.

// Access through 3 data-test-subj
const menu = testBed.find('ccrApp.myTable.menu'); // returns a react wrapper
wayneseymour commented 5 years ago

@sebelga can the functional tests benefit from integrating your test bed into them? If so, how does that work?

sebelga commented 5 years ago

@wayneseymour, unfortunately, the Testbed is currently coupled with the react enzyme testing library and has a few helpers that would not make sense at the functional level (mounting component, wrap a router around them, ...). But some part of it could be easily rewritten to help functional testing (like being able to target a DOM node with a dot notation for example or read the content of a table).

wayneseymour commented 5 years ago

Hrmm, ok. I'd really like to get this Testbed integrated more, seems useful even if not so much on the functional side.

On Thu, Jun 13, 2019 at 11:26 PM Sébastien Loix notifications@github.com wrote:

@wayneseymour https://github.com/wayneseymour, unfortunately, the Testbed is currently coupled with the react enzyme testing library and has a few helpers that would not make sense at the functional level (mounting component, wrap a router around them, ...). But some part of it could be easily rewritten to help functional testing (like being able to target a DOM node with a dot notation for example or read the content of a table).

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/elastic/kibana/issues/37698?email_source=notifications&email_token=ABFCBQ2VEE7OH24P74DA74DP2MTXPA5CNFSM4HR4SYX2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXVX5DY#issuecomment-501972623, or mute the thread https://github.com/notifications/unsubscribe-auth/ABFCBQZP7HCO7VRUUDPRFX3P2MTXPANCNFSM4HR4SYXQ .

--

Tre' Seymour

Senior Software Engineer

Kibana QA - JavaScript Engineer in Test

https://www.elastic.co/