[ILM] Refactor edit_policy client integration tests into separate feature files by yuliacech · Pull Request #92826 · elastic/kibana

yuliacech · 2021-02-25T15:25:27Z

Addresses #88593
Continues work started in PR#91657

Summary

This PR refactors the 2nd large edit_policy.test.ts file into separate files categorized by features of the form.

Folder structure before

edit_policy
├── constants.ts
├── edit_policy.helpers.tsx
├── edit_policy.test.ts
├── form_validation
│   ├── cold_phase_validation.test.ts
│   ├── delete_phase_validation.ts
│   ├── hot_phase_validation.test.ts
│   ├── policy_name_validation.test.ts
│   └── warm_phase_validation.test.ts
└── reactive_form
    ├── node_allocation.test.ts
    └── reactive_form.test.ts

Folder structure after

edit_policy
├── constants.ts
├── edit_policy.helpers.tsx
├── features
│   ├── cold_phase.test.ts
│   ├── delete_phase.test.ts
│   ├── json_flyout.test.ts
│   ├── node_allocation.test.ts
│   ├── rollover.test.ts
│   ├── searchable_snapshots.test.ts
│   ├── timeline.test.ts
│   └── warm_phase.test.ts
├── form_validation
│   ├── cold_phase_validation.test.ts
│   ├── delete_phase_validation.ts
│   ├── error_indicators.test.ts
│   ├── hot_phase_validation.test.ts
│   ├── policy_name_validation.test.ts
│   └── warm_phase_validation.test.ts
└── serialization
    └── policy_serialization.test.ts

Description of changes

Added error indicators tests to form_validation folder
Split reactive_form.test.ts into smaller files and renamed reactive_form folder to features that now contains:
- warm phase tests
- cold phase tests
- delete phase tests
- node allocation tests
- request flyout tests
- rollover tests
- searchable snapshots tests
- timeline tests
Added serialization folder with serialization tests

Future work

Another improvement should be done for helpers file that is has now already reached ~400 lines. Now that we have separated tests into features, some helper methods can be moved to a corresponding file where they are used leaving the general helpers file small and readable.
Many small files have a downside of repeating boilerplate of setting up the testbed. Test bed setup function could be more reusable with some defaults to avoid code duplication.

elasticmachine · 2021-02-25T16:32:54Z

Pinging @elastic/es-ui (Team:Elasticsearch UI)

cjcenizal · 2021-02-25T16:46:03Z

@yuliacech Could you collaborate with @cuff-links and get his input on the readability and scannability of these tests? In our retro we identified a need to better coordinate our automated test coverage with our manual test coverage. I think one step in this direction will be to make it easier for QA engineers to understand the UX flows being tested by our automated tests, so they can compare them against the UX flows covered by TestRail. I think this will be a long-term process so no need to get it right in this particular PR, but I think this is a good opportunity to start gathering this feedback. WDYT?

yuliacech · 2021-02-25T16:54:58Z

Thanks for pointing this out, @cjcenizal! I'll make sure to get @cuff-links input and feedback for this.

jloleysens

This is looking really great @yuliacech ! 🎉 The new structure is really cool.

I agree with CJ's comment regarding readability of tests, to my mind the thing that could use more careful thinking through is how the describe and test names serialize into something really clear for readers of the test results. At least, I know I could do better here! I think that consistency is of primary importance but I'd be interested to hear from @cuff-links (and you) too! Not sure we need to block this PR on that though - we can always do another pass.

Would you mind converting the PR description to use ASCII-style tree (generated with CLI tree) for the new test dir structure:

client_integration
├── app
│   ├── app.helpers.tsx
│   └── app.test.ts
├── edit_policy
│   ├── constants.ts
│   ├── edit_policy.helpers.tsx
│   ├── edit_policy.test.ts
│   ├── form_validation
│   │   ├── cold_phase_validation.test.ts
│   │   ├── delete_phase_validation.ts
│   │   ├── hot_phase_validation.test.ts
│   │   ├── policy_name_validation.test.ts
│   │   └── warm_phase_validation.test.ts
│   └── reactive_form
│       ├── node_allocation.test.ts
│       └── reactive_form.test.ts
└── helpers
    ├── http_requests.ts
    ├── index.ts
    └── setup_environment.ts

5 directories, 15 files

This would really help me to grok the structural changes introduced in this PR!

The main thing I'd like to get your thoughts on for my review, before approving (because mostly this looks pretty great to me!!) is the the reactive_form.test.ts file. At the moment my concern is that it is not 100% clear when I should add something here vs another feature test file. This can be addressed by a comment or README.md to capture the constraints of when to add something there. Alternatively, the tests in reactive_form.test.ts could be moved to separate or existing feature test files. For instance; min age tooltip could be moved to rollover file, JSON flyout could be its own feature and the special delete phase behaviour could also be its own file. Let me know what you think of one of those two directions!

…iles

yuliacech · 2021-03-01T15:18:14Z

@elasticmachine merge upstream

kibanamachine · 2021-03-01T15:18:16Z

merge conflict between base and head

yuliacech · 2021-03-01T17:12:12Z

Thanks a lot for your review, @jloleysens! I agree about reactive_form file, I thought that the name is not very clear. So I split it into smaller files as you suggested. Also, great idea for the folder structure representation, I've haven't used tree before and it's really useful.

kibanamachine · 2021-03-01T19:03:44Z

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

💚 Build #109995 succeeded 934ce2d
💚 Build #109940 succeeded f75fc23
💚 Build #109453 succeeded 16e4e5016694dd94e9c559e47f60d847db523b41
💚 Build #109421 succeeded 1c324d0e5846e7efe2716f64cc17f086ba9ead43

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

jloleysens

Thanks for making those changes @yuliacech ! Happy for this to be merged!

yuliacech · 2021-03-02T14:20:00Z

I've merged this PR for now and will plan a feedback session with @cuff-links when he's back.

…ture files (elastic#92826) * Refactor edit_policy client integration tests into separate feature files * Fixed merge conflicts with master * Updated rollover tests to match master branch code * Split reactive_form into smaller files * Renamed flyout tests file

…bana into task-manager/docs-monitoring * 'task-manager/docs-monitoring' of github.com:gmmorris/kibana: [ILM] Allow multiple searchable snapshot actions (elastic#92789) Improve consistency for display of management items (elastic#92694) skip flaky suite (elastic#93152) skip flaky suite (elastic#93152) [ILM] Refactor edit_policy client integration tests into separate feature files (elastic#92826) Add developer documentation about the building blocks we offer plugin developers (elastic#92743) [Security Solution] Case ui enhancement (elastic#91863) [Security Solution] [Detections] Updates warning message when no indices match provided index patterns (elastic#93094) Collect agent telemetry even when fleet server is disabled. (elastic#93198) [Lens] Fix runtime validation error message (elastic#93195) [Lens] Remove warning about ordinal x-domain (elastic#93049) [Security Solution] Fixes the Customize Event Renderers modal by removing the EuiOverlayMask (elastic#93150) Cleanup Security plugin imports (elastic#93056) [Security Solution] - Bug fixes (elastic#92294) Updated doc links (elastic#92968) [ML] Transforms: Fixes chart histograms for runtime fields. (elastic#93028) [chore] Enable core's eslint rule: `@ts-expect-error` (elastic#93086)

…ture files (#92826) (#93229) * Refactor edit_policy client integration tests into separate feature files * Fixed merge conflicts with master * Updated rollover tests to match master branch code * Split reactive_form into smaller files * Renamed flyout tests file

…ture files (#92826) (#93228) * Refactor edit_policy client integration tests into separate feature files * Fixed merge conflicts with master * Updated rollover tests to match master branch code * Split reactive_form into smaller files * Renamed flyout tests file

* master: (199 commits) Convert Canvas docs to MDX for use in Elastic Docs (elastic#91969) [Bazel] More resilient Workspace Status (elastic#93244) [Discover] Change icon of saved search in open search panel and embeddable selection (elastic#93001) [Workplace Search] Role Mappings to Kibana (elastic#93123) [Fleet] Use type-only imports where possible (elastic#92979) [Lens] Set pie chart slices sorted clockwise (elastic#92617) Remove ms label from CPU load on status page (elastic#92836) [App Search] Migrate Create Meta Engine View (elastic#92127) [Time to Visualize] Disable Visualize URL Tracker When Linked to OriginatingApp (elastic#92917) [ILM] Allow multiple searchable snapshot actions (elastic#92789) Improve consistency for display of management items (elastic#92694) skip flaky suite (elastic#93152) skip flaky suite (elastic#93152) [ILM] Refactor edit_policy client integration tests into separate feature files (elastic#92826) Add developer documentation about the building blocks we offer plugin developers (elastic#92743) [Security Solution] Case ui enhancement (elastic#91863) [Security Solution] [Detections] Updates warning message when no indices match provided index patterns (elastic#93094) Collect agent telemetry even when fleet server is disabled. (elastic#93198) [Lens] Fix runtime validation error message (elastic#93195) [Lens] Remove warning about ordinal x-domain (elastic#93049) ...

yuliacech added Feature:ILM release_note:skip Skip the PR/issue when compiling release notes Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more t// v7.12.0 v7.13.0 v8.0.0 labels Feb 25, 2021

yuliacech force-pushed the ilm_tests_refactor branch from 1c324d0 to 16e4e50 Compare February 25, 2021 16:22

yuliacech marked this pull request as ready for review February 25, 2021 16:32

yuliacech requested a review from a team as a code owner February 25, 2021 16:32

yuliacech requested a review from jloleysens February 25, 2021 16:33

yuliacech requested a review from cuff-links February 25, 2021 16:55

jloleysens reviewed Mar 1, 2021

View reviewed changes

yuliacech added 2 commits March 1, 2021 16:14

Refactor edit_policy client integration tests into separate feature f…

7bd0b50

…iles

Fixed merge conflicts with master

f0a7380

yuliacech marked this pull request as draft March 1, 2021 15:20

yuliacech force-pushed the ilm_tests_refactor branch from 16e4e50 to f0a7380 Compare March 1, 2021 15:21

Updated rollover tests to match master branch code

f75fc23

yuliacech marked this pull request as ready for review March 1, 2021 15:28

yuliacech added 2 commits March 1, 2021 17:56

Split reactive_form into smaller files

934ce2d

Renamed flyout tests file

e014739

jloleysens approved these changes Mar 2, 2021

View reviewed changes

yuliacech merged commit bf9517e into elastic:master Mar 2, 2021

This was referenced Mar 2, 2021

[7.x] [ILM] Refactor edit_policy client integration tests into separate feature files (#92826) #93228

Merged

[7.12] [ILM] Refactor edit_policy client integration tests into separate feature files (#92826) #93229

Merged

yuliacech deleted the ilm_tests_refactor branch March 5, 2021 11:57

yuliacech mentioned this pull request Apr 21, 2021

[ILM] Refactor client integration tests #88593

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ILM] Refactor edit_policy client integration tests into separate feature files#92826

[ILM] Refactor edit_policy client integration tests into separate feature files#92826
yuliacech merged 5 commits intoelastic:masterfrom
yuliacech:ilm_tests_refactor

yuliacech commented Feb 25, 2021 •

edited

Loading

Uh oh!

elasticmachine commented Feb 25, 2021

Uh oh!

cjcenizal commented Feb 25, 2021

Uh oh!

yuliacech commented Feb 25, 2021

Uh oh!

jloleysens left a comment

Uh oh!

yuliacech commented Mar 1, 2021

Uh oh!

kibanamachine commented Mar 1, 2021

Uh oh!

yuliacech commented Mar 1, 2021

Uh oh!

kibanamachine commented Mar 1, 2021

Uh oh!

jloleysens left a comment

Uh oh!

yuliacech commented Mar 2, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

yuliacech commented Feb 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Folder structure before

Folder structure after

Description of changes

Future work

Uh oh!

elasticmachine commented Feb 25, 2021

Uh oh!

cjcenizal commented Feb 25, 2021

Uh oh!

yuliacech commented Feb 25, 2021

Uh oh!

jloleysens left a comment

Choose a reason for hiding this comment

Uh oh!

yuliacech commented Mar 1, 2021

Uh oh!

kibanamachine commented Mar 1, 2021

Uh oh!

yuliacech commented Mar 1, 2021

Uh oh!

kibanamachine commented Mar 1, 2021

💚 Build Succeeded

Metrics [docs]

History

Uh oh!

jloleysens left a comment

Choose a reason for hiding this comment

Uh oh!

yuliacech commented Mar 2, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

yuliacech commented Feb 25, 2021 •

edited

Loading