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

[ILM] Refactor component tests #91657

Merged
merged 2 commits into from
Feb 19, 2021
Merged

[ILM] Refactor component tests #91657

merged 2 commits into from
Feb 19, 2021

Conversation

yuliacech
Copy link
Contributor

@yuliacech yuliacech commented Feb 17, 2021

Related to #88593

Summary

This PR refactors component integration tests for edit policy page and updates them to use client integration tests. Client integration tests simulate a larger context in which the component is being used in the app than component integration tests.
A single file with component integration tests for edit policy is now separated into categories:

  • Form validation:

    • policy name validation
    • hot / warm / cold / delete phase validation
  • Reactive form (how form hides/shows certain controls based on user input or context)

    • node allocation tests
    • phases tests

Future work

In a follow up PR I'd like to separate remaining client integration tests from a large file (~ 900 lines) into created categories and add new ones if needed.

@yuliacech yuliacech added chore 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 v8.0.0 v7.13.0 labels Feb 17, 2021
@yuliacech yuliacech marked this pull request as ready for review February 18, 2021 11:09
@yuliacech yuliacech requested a review from a team as a code owner February 18, 2021 11:09
@elasticmachine
Copy link
Contributor

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

@yuliacech yuliacech requested a review from jloleysens February 18, 2021 11:09
Copy link
Contributor

@jloleysens jloleysens left a comment

Choose a reason for hiding this comment

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

Great work @yuliacech ! I am really happy to see the old components folder on its way out!

I left a few comments that I'd like to get your thoughts on moving forward. I don't think any of them are worth blocking legacy migration for 🍻

type Phases = keyof PolicyPhases;

import { POLICY_NAME } from './constants';
import { TestSubjects } from '../helpers';
window.scrollTo = jest.fn();
Copy link
Contributor

Choose a reason for hiding this comment

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

This should no longer be necessary, if it is then we probably need to remove this from the code because AFAIK this is used to scroll to the first visible error on form submission. It's nice behaviour, but has not been mapped onto the new UI with collapsible settings in phases.

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 still use a scroll to 0,0 for when the policy has loaded (edit_policy.tsx#L66). It doesn't affect the tests results though, but I prefer to keep it there for now to avoid unnecessary errors in output.

@@ -127,7 +125,7 @@ describe('<EditPolicy />', () => {
await actions.hot.setBestCompression(true);
await actions.hot.toggleShrink(true);
await actions.hot.setShrink('2');
await actions.hot.setReadonly(true);
await actions.hot.toggleReadonly(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

I've realised that these toggle/set functions do not actually set the boolean value, they just toggle whatever the toggle state is relative to what it was before. Not something to action, just observing since I introduced the boolean arg here.

Might just need some slight investigation to fix, @sebelga have you encountered this?

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 pointing this out @jloleysens! It's worth investigating for sure, I'll put it in an issue.

@@ -0,0 +1,143 @@
/*
Copy link
Contributor

@jloleysens jloleysens Feb 18, 2021

Choose a reason for hiding this comment

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

I like the direction this is heading in, it is an important category of form states to capture in tests as something distinct from serialising or validation.

I would ask, though, how this file will scale if new functionality or new tests for old functionality is added? Do we append to this file?

One way that I was thinking about this is as "workflows", but I have since come think of it as "features" of the UI. The UI has a feature for viewing JSON in a flyout, which is one thing, it also has a feature for helping users configure rollover.

Anyway, just throwing these out there! Let me know what you think!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think 'features' is a good system to try and separate tests further. My plan is to address the second large edit_policy.test.ts file in a follow up PR and probably introduce those categories into reactive_form folder.

@yuliacech
Copy link
Contributor Author

@elasticmachine merge upstream

@yuliacech
Copy link
Contributor Author

Thanks a lot for your review, @jloleysens! I'm so looking forward to merging this and getting rid of at least 1 huge test file :)

@yuliacech yuliacech merged commit f8fd08f into elastic:master Feb 19, 2021
@yuliacech
Copy link
Contributor Author

yuliacech commented Feb 19, 2021

Hi @cjcenizal, what do you think about backporting this to 7.12 even after FF? I think it could very useful for any future bug fixing in 7.12 branch and the changes only touch test files (low risk).

yuliacech added a commit to yuliacech/kibana that referenced this pull request Feb 19, 2021
…g client integration testing setup (elastic#91657)

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Feb 19, 2021
* master: (111 commits)
  [Logs UI] Replace dependencies in the infra bundle (elastic#91503)
  [Search Source] Do not request unmapped fields if source filters are provided (elastic#91921)
  [APM] Kql Search Bar suggests values outside the selected time range (elastic#91918)
  Refactored component edit policy tests into separate folders and using client integration testing setup (elastic#91657)
  [Fleet] Don't error on missing package_assets value (elastic#91744)
  [Lens] Pass used histogram interval to chart (elastic#91370)
  [Indexpattern management] Use indexPatterns Service instead of savedObjects client (elastic#91839)
  [Security Solutions] Fixes Cypress tests for indicator match by making the selectors more specific (elastic#91947)
  [CI] backportrc can skip CI (elastic#91886)
  Revert "[SOM] fix flaky suites (elastic#91809)"
  [Fleet] Install Elastic Agent integration by default during setup (elastic#91676)
  [Fleet] Silently swallow 404 errors when deleting ingest pipelines (elastic#91778)
  [data.search] Use incrementCounter for search telemetry (elastic#91230)
  [Fleet] Bootstrap functional test suite (elastic#91898)
  [Alerts][Docs] Added API documentation for alerts plugin (elastic#91067)
  Use correct environment in anomaly detection setup link (elastic#91877)
  [FTSR] Convert to tasks and add jest/api integration suites (elastic#91770)
  [CI] Build and publish storybooks (elastic#87701)
  docs: add PHP agent info to docs (elastic#91773)
  [DOCS] Adds and updates Visualization advanced settings (elastic#91904)
  ...
@cjcenizal
Copy link
Contributor

@yuliacech Good call! I agree with your reasoning.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

yuliacech added a commit to yuliacech/kibana that referenced this pull request Feb 19, 2021
…g client integration testing setup (elastic#91657)

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
yuliacech added a commit that referenced this pull request Feb 19, 2021
…g client integration testing setup (#91657) (#92020)

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
yuliacech added a commit that referenced this pull request Feb 22, 2021
…g client integration testing setup (#91657) (#92045)

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@yuliacech yuliacech deleted the ilm_tests branch February 24, 2021 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore 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 v7.13.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants