Added tests for dot notation processors.

[cuff-links]

Summary

This PR is to add test coverage for the Ingest Pipelines Date Processor.

Test cases being covered

Prevents submission 0/1 required fields given.
Prevents submission when field does not contain . for the dot notation.
Saves processor with default options
Saves processor with setting optional options.

[elasticmachine]

Pinging @elastic/kibana-stack-management (Team:Stack Management)

[cuff-links]

@sebelga For the second test in this test case, when I fill out the field but omit the . for the dot notation, the error does not show up in the errors object in the assertion when I save the processor. It does when I test manually though. I am not sure if this is an issue with the test, forms lib, bug, or what have you. Can you try this out? I tried loggin component.debug() but the output was so large that I got lost in it.

[sebelga]

As you are testing a validation to run and then display a message you need to wait for the validation to have ran. The validation run after a 500ms debounce time after the form input value has change.

As you correctly use the fake timer from jest the 500ms never pass and the validation does not run.

This is what you need to do

// Wrap the `setInputValue` in an `act()` call as it modifies the internal value state of the field and mark it as "changed".
// This trigger the debounce which now waits... 
await act(async () => {
  form.setInputValue('fieldNameField.input', 'missing');
});
// Move ahead the debounce time which will then execute any validations
await act(async () => {
  jest.runAllTimers();
});

[alisonelizabeth]

@sebelga interestingly enough - we are not wrapping other instances of form.setInputValue() in act() (although I guess we probably should be?) or calling runAllTimers() in these processor tests. Is there something different about this specific validation?

On another note - should the act() be part of the actual setInputValue method in the testbed utils? I see it does call component.update() after the simulate change - related code.

[sebelga]

Depending on what you are testing and if there is code reacts to the field value change. You might not need to wrap with an act when changing the form input in certain cases. Here he is testing the result of the validation so he both needs the act() and wait for the debounce.

If for example you are testing that the correct payload is sent, then you don't need to wait for useEffect (that triggers the validate() on field). You can set the value of all fields synchronously and simulate the click of the "Send" button. That will call the form.getData() and all the values are there synchronously.

As you can see it in my latest test for preview, my helpers to modify field values all wrap with an act call (https://github.com/elastic/kibana/pull/106410/files#diff-8733b59b04f21f21eb6bf66bd48aba1334abad4ec93d8bde99ac1386e000f131R32)

should the act() be part of the actual setInputValue method in the testbed utils?

It probably would have be good to do it there. Now it would require a lot of work to add it there and then remove it everywhere we have wrapped setInputValue with an act as it would be wrapped twice 😊

Note that this is all due to hooks and the way state updates and react in useEffect. The await act( ... ) makes sure all the lifecycle has ran.

[alisonelizabeth]

Thanks for the explanation @sebelga!

I was thinking there were other test cases where we were verifying the input validation, but I see now most of the processor tests are only testing empty fields on submit, e.g.,

 test('prevents form submission if required fields are not provided', async () => {
    const {
      actions: { saveNewProcessor },
      form,
    } = testBed;

    // Click submit button with only the type defined
    await saveNewProcessor();

    // Expect form error as "field" and "target_field" are required parameters
    expect(form.getErrorsMessages()).toEqual(['A field value is required.']);
  });

[sebelga]

Yes and saveNewProcessor is wrapped

async saveNewProcessor() {
  await act(async () => {
    find('addProcessorForm.submitButton').simulate('click');
  });
  component.update();
}

[cuff-links]

I get it. I am not sure if this is spelled out explicitly in our CIT docs but I think it would be useful to do so. @sebelga @alisonelizabeth

[alisonelizabeth]

Nice work @cuff-links! Left a few nits - looks like a some of the comments in the tests might have been copy-pasted and need to be updated.

[alisonelizabeth]

@sebelga interestingly enough - we are not wrapping other instances of form.setInputValue() in act() (although I guess we probably should be?) or calling runAllTimers() in these processor tests. Is there something different about this specific validation?

On another note - should the act() be part of the actual setInputValue method in the testbed utils? I see it does call component.update() after the simulate change - related code.

[cjcenizal]

@cuff-links We're in the 7.15 dev cycle now, so could you please update the version labels?

[alisonelizabeth]

Latest LGTM. Ran tests locally. Thanks @cuff-links!

Looks like there are some linting issues that may need to be addressed before merging. The test failure reported looks unrelated to these changes.

Also, I noticed there is a new parameter for the dot expander processor. Opened #108895 as a follow-up issue to address.

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: c9ddb06
• Storybooks Preview
• Documentation Changes

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
ingestPipelines	709.7KB	709.8KB	+29.0B

History

• 💔 Build #146004 failed c1ad0f4
• 💔 Build #142109 failed 9873706

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

[kibanamachine]

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.