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.

Sign up for GitHub

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

Jump to bottom

Timelines Plugin Tech Debt Cleanup #111581

Closed
7 tasks
kqualters-elastic opened this issue Sep 8, 2021 · 2 comments
Closed
7 tasks

Timelines Plugin Tech Debt Cleanup #111581

kqualters-elastic opened this issue Sep 8, 2021 · 2 comments
Assignees
@XavierM @andrew-goldstein @kqualters-elastic
Labels
Feature:RAC label obsolete Feature:Timeline Security Solution Timeline feature Meta Team:Threat Hunting Security Solution Threat Hunting Team

Comments

@kqualters-elastic
Copy link
Contributor

kqualters-elastic commented Sep 8, 2021 •
edited by michaelolo24
Loading

As part of the Rules, Alerts, and Cases effort in 7.14/7.15, a new timelines plugin was created to share the alerts table and derivative workflows originally found in the security solution plugin. This issue will serve as a meta issue to track the effort to cleanup some of the tech debt incurred during this effort.

  • Move or remove the various search strategies defined(and sometimes duplicated in security solution) in the timelines plugin back to security solution, and replace with new, simplified alerting based search strategies for use with the alerts table. @XavierM
  • Remove any known duplicated code in timelines/cases/security solution. @XavierM
  • Make use of the kibana wide ECS schema and not the ad hoc security solution ECS implementation. @XavierM
  • Move the add to new/existing case markup and logic from timelines to the cases plugin. @kqualters-elastic
  • Remove cases logic from timelines redux state. Use callbacks instead @kqualters-elastic
  • Add unit/integration tests for any code paths currently lacking them. @andrew-goldstein
  • Reduce timelines bundle size below 100kb. Initially via removing unneeded code in the timelines plugin, and then by making better use of React.load and async imports.
@kqualters-elastic kqualters-elastic added Meta Team:Threat Hunting Security Solution Threat Hunting Team Feature:Timeline Security Solution Timeline feature Feature:RAC label obsolete 7.16 candidate labels Sep 8, 2021
@kqualters-elastic kqualters-elastic self-assigned this Sep 8, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-threat-hunting (Team:Threat Hunting)

@michaelolo24 michaelolo24 mentioned this issue Sep 14, 2021
Closed
@michaelolo24 michaelolo24 mentioned this issue Sep 22, 2021
Open
@andrew-goldstein andrew-goldstein mentioned this issue Oct 1, 2021
Merged
andrew-goldstein added a commit to andrew-goldstein/kibana that referenced this issue Oct 6, 2021
@andrew-goldstein
## [Security Solution] Increases code coverage in the timelines plugin
5813f0d 
This PR is the first in a series that increases code coverage in the `timelines` plugin, as part of <elastic#111581>

This PR is being opened in `[Draft]` status to facilitate a team discussion re: React Testing Library vs Enzyme, as outlined in the description below.

### Methodology

1. Code coverage is measured by running the following command:

```
cd $KIBANA_HOME/x-pack && node scripts/jest.js timelines --coverage
```

The above command outputs the following coverage report:

```
kibana/target/kibana-coverage/jest/index.html
```

2. The coverage report is used to determine which paths need coverage, and measure coverage before / after tests are updated, as illustrated by the screenshots below:

**Before (example)**

![file-summary-before](https://user-images.githubusercontent.com/4459398/135690108-f90839b1-1450-4083-b928-5c5d99f1151d.png)

![file-coverage-before](https://user-images.githubusercontent.com/4459398/135690178-be24e716-545f-425f-bcd5-480026fcad1f.png)

**After (example)**

![file-summary-after](https://user-images.githubusercontent.com/4459398/135690267-7e94655f-4852-42f7-8180-8c195dd77e8b.png)

![file-coverage-after](https://user-images.githubusercontent.com/4459398/135690232-63130180-3fa1-4989-ac69-d8af7cc8fc95.png)

### React Testing Library vs Enzyme

- New test files are created using [React Testing Library](https://github.com/testing-library/react-testing-library) by default

- [Enzyme](https://github.com/enzymejs/enzyme) tests will only be used as a fallback when it's not reasonably possible to express the test in React Testing Library

- To align with the Kibana [STYLEGUIDE](https://github.com/elastic/kibana/blob/master/STYLEGUIDE.mdx#camel-case-id-and-data-test-subj), for now, the React Testing Library [getByTestId](https://testing-library.com/docs/queries/bytestid/) API is used in most tests, even though it has [a lower priority](https://testing-library.com/docs/queries/about#priority).

- Jest was already configured to use the `getByTestId` API with `data-test-subj` [here](https://github.com/elastic/kibana/blob/4a541883557bcee83baf09b2c0ddab702f780e45/packages/kbn-test/src/jest/setup/react_testing_library.js#L20)
andrew-goldstein added a commit that referenced this issue Oct 6, 2021
@andrew-goldstein
[Security Solution] Increases code coverage in the timelines plugin (…
5306632 
…#113681)

## [Security Solution] Increases code coverage in the `timelines` plugin

This PR is the first in a series that increases code coverage in the `timelines` plugin, as part of <#111581>

### Methodology

1. Code coverage is measured by running the following command:

```
cd $KIBANA_HOME/x-pack && node scripts/jest.js timelines --coverage
```

The above command outputs the following coverage report:

```
kibana/target/kibana-coverage/jest/index.html
```

2. The coverage report is used to determine which paths need coverage, and measure coverage before / after tests are updated, as illustrated by the screenshots below:

**Before (example)**

![file-summary-before](https://user-images.githubusercontent.com/4459398/135690108-f90839b1-1450-4083-b928-5c5d99f1151d.png)

![file-coverage-before](https://user-images.githubusercontent.com/4459398/135690178-be24e716-545f-425f-bcd5-480026fcad1f.png)

**After (example)**

![file-summary-after](https://user-images.githubusercontent.com/4459398/135690267-7e94655f-4852-42f7-8180-8c195dd77e8b.png)

![file-coverage-after](https://user-images.githubusercontent.com/4459398/135690232-63130180-3fa1-4989-ac69-d8af7cc8fc95.png)

### React Testing Library vs Enzyme

- New test files are created using [React Testing Library](https://github.com/testing-library/react-testing-library) by default

- [Enzyme](https://github.com/enzymejs/enzyme) tests will only be used as a fallback when it's not reasonably possible to express the test in React Testing Library

- Code will (still) be instrumented to use `data-test-subj` in alignment with the Kibana [STYLEGUIDE](https://github.com/elastic/kibana/blob/master/STYLEGUIDE.mdx#camel-case-id-and-data-test-subj)

- When possible, the `getByRole` and other [higher priority](https://testing-library.com/docs/queries/about#priority) query APIs will be used in Jest tests, as opposed to selecting via `getByTestId` + `data-test-subj`. This follows the [guidance from React Testing Library](https://testing-library.com/docs/queries/about#priority).

- Note: Jest was already configured to use the `getByTestId` API with `data-test-subj` [here](https://github.com/elastic/kibana/blob/4a541883557bcee83baf09b2c0ddab702f780e45/packages/kbn-test/src/jest/setup/react_testing_library.js#L20)
kibanamachine pushed a commit to kibanamachine/kibana that referenced this issue Oct 6, 2021
@andrew-goldstein @kibanamachine
[Security Solution] Increases code coverage in the timelines plugin (…
989f0da 
…elastic#113681)

## [Security Solution] Increases code coverage in the `timelines` plugin

This PR is the first in a series that increases code coverage in the `timelines` plugin, as part of <elastic#111581>

### Methodology

1. Code coverage is measured by running the following command:

```
cd $KIBANA_HOME/x-pack && node scripts/jest.js timelines --coverage
```

The above command outputs the following coverage report:

```
kibana/target/kibana-coverage/jest/index.html
```

2. The coverage report is used to determine which paths need coverage, and measure coverage before / after tests are updated, as illustrated by the screenshots below:

**Before (example)**

![file-summary-before](https://user-images.githubusercontent.com/4459398/135690108-f90839b1-1450-4083-b928-5c5d99f1151d.png)

![file-coverage-before](https://user-images.githubusercontent.com/4459398/135690178-be24e716-545f-425f-bcd5-480026fcad1f.png)

**After (example)**

![file-summary-after](https://user-images.githubusercontent.com/4459398/135690267-7e94655f-4852-42f7-8180-8c195dd77e8b.png)

![file-coverage-after](https://user-images.githubusercontent.com/4459398/135690232-63130180-3fa1-4989-ac69-d8af7cc8fc95.png)

### React Testing Library vs Enzyme

- New test files are created using [React Testing Library](https://github.com/testing-library/react-testing-library) by default

- [Enzyme](https://github.com/enzymejs/enzyme) tests will only be used as a fallback when it's not reasonably possible to express the test in React Testing Library

- Code will (still) be instrumented to use `data-test-subj` in alignment with the Kibana [STYLEGUIDE](https://github.com/elastic/kibana/blob/master/STYLEGUIDE.mdx#camel-case-id-and-data-test-subj)

- When possible, the `getByRole` and other [higher priority](https://testing-library.com/docs/queries/about#priority) query APIs will be used in Jest tests, as opposed to selecting via `getByTestId` + `data-test-subj`. This follows the [guidance from React Testing Library](https://testing-library.com/docs/queries/about#priority).

- Note: Jest was already configured to use the `getByTestId` API with `data-test-subj` [here](https://github.com/elastic/kibana/blob/4a541883557bcee83baf09b2c0ddab702f780e45/packages/kbn-test/src/jest/setup/react_testing_library.js#L20)
kibanamachine added a commit that referenced this issue Oct 7, 2021
@kibanamachine @andrew-goldstein
[Security Solution] Increases code coverage in the timelines plugin (…
f27ec3f 
…#113681) (#114205)

## [Security Solution] Increases code coverage in the `timelines` plugin

This PR is the first in a series that increases code coverage in the `timelines` plugin, as part of <#111581>

### Methodology

1. Code coverage is measured by running the following command:

```
cd $KIBANA_HOME/x-pack && node scripts/jest.js timelines --coverage
```

The above command outputs the following coverage report:

```
kibana/target/kibana-coverage/jest/index.html
```

2. The coverage report is used to determine which paths need coverage, and measure coverage before / after tests are updated, as illustrated by the screenshots below:

**Before (example)**

![file-summary-before](https://user-images.githubusercontent.com/4459398/135690108-f90839b1-1450-4083-b928-5c5d99f1151d.png)

![file-coverage-before](https://user-images.githubusercontent.com/4459398/135690178-be24e716-545f-425f-bcd5-480026fcad1f.png)

**After (example)**

![file-summary-after](https://user-images.githubusercontent.com/4459398/135690267-7e94655f-4852-42f7-8180-8c195dd77e8b.png)

![file-coverage-after](https://user-images.githubusercontent.com/4459398/135690232-63130180-3fa1-4989-ac69-d8af7cc8fc95.png)

### React Testing Library vs Enzyme

- New test files are created using [React Testing Library](https://github.com/testing-library/react-testing-library) by default

- [Enzyme](https://github.com/enzymejs/enzyme) tests will only be used as a fallback when it's not reasonably possible to express the test in React Testing Library

- Code will (still) be instrumented to use `data-test-subj` in alignment with the Kibana [STYLEGUIDE](https://github.com/elastic/kibana/blob/master/STYLEGUIDE.mdx#camel-case-id-and-data-test-subj)

- When possible, the `getByRole` and other [higher priority](https://testing-library.com/docs/queries/about#priority) query APIs will be used in Jest tests, as opposed to selecting via `getByTestId` + `data-test-subj`. This follows the [guidance from React Testing Library](https://testing-library.com/docs/queries/about#priority).

- Note: Jest was already configured to use the `getByTestId` API with `data-test-subj` [here](https://github.com/elastic/kibana/blob/4a541883557bcee83baf09b2c0ddab702f780e45/packages/kbn-test/src/jest/setup/react_testing_library.js#L20)

Co-authored-by: Andrew Goldstein <andrew-goldstein@users.noreply.github.com>
@michaelolo24
Copy link
Contributor

Closing in favor of: https://github.com/elastic/security-team/issues/2441

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@XavierM XavierM

@andrew-goldstein andrew-goldstein

@kqualters-elastic kqualters-elastic

Labels
Feature:RAC label obsolete Feature:Timeline Security Solution Timeline feature Meta Team:Threat Hunting Security Solution Threat Hunting Team
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@XavierM @andrew-goldstein @elasticmachine @michaelolo24 @kqualters-elastic