[Rollup] Fix component integration tests #121144
Conversation
sebelga
added
Team:Kibana Management
|
Pinging @elastic/kibana-stack-management (Team:Stack Management) |
| @@ -159,7 +159,7 @@ export class JobCreateUi extends Component { | |||
| requestIndexPatternValidation = debounce((resetDefaults = true) => { | |||
| const indexPattern = this.getIndexPattern(); | |||
|
|
|||
| const lastIndexPatternValidationTime = (this.lastIndexPatternValidationTime = Date.now()); | |||
| const lastIndexPatternValidationIdx = ++this.lastIndexPatternValidationIdx; | |||
There was a problem hiding this comment.
Not relying on the Date for this as we can simply use an incremental counter. Better for the tests too.
sebelga
added
the
release_note:skip
|
@elasticmachine merge upstream |
💚 Build Succeeded
Metrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: |
| form.setInputValue('rollupJobName', JOB_TO_CREATE.id); | ||
| await form.setInputValue('rollupIndexPattern', JOB_TO_CREATE.indexPattern, true); | ||
| form.setInputValue('rollupIndexName', JOB_TO_CREATE.rollupIndex); | ||
| act(() => { |
There was a problem hiding this comment.
I'm not quite sure what's the difference between await act(async () => { and this version without await and async?
There was a problem hiding this comment.
Ideally we want to be as synchronous as possible and use act(). In some cases using the synchronous version is not enough because, for example, clicking a button triggers a promise that resolves on the next tick and then updates the state. The way to wait for that next tick is to use the async version await act(async() => {...}).
I usually start with the sync version and if the state does not update I change to the async one. With component integration tests it is not always easy to remember all the moving parts occuring after changing a field value or clicking a button.
| @@ -29,11 +26,16 @@ describe('Cloning a rollup job through create job wizard', () => { | |||
| let startMock; | |||
|
|
|||
| beforeAll(() => { | |||
| jest.useFakeTimers(); | |||
There was a problem hiding this comment.
I'm wondering why we would need fakeTimers if we don't use any of advanceTimersByTime or runAllTimers, would you mind explaining?
There was a problem hiding this comment.
We want to have the guarantee that there aren't any timeout running in the tests. We are not using jest for isolated unit tests but for component integration tests which load all the npm dependencies which could potentially use timeouts. And we don't want them in our test.
By adding this on top of our tests we are removing any risk of timing out so I would recommend to declare jest.useFakeTimers(); to every component integration test file.
| await new Promise((res) => setTimeout(res, 750)); | ||
|
|
||
| // There is a 500 timeout before receiving the response. | ||
| // To be investigated, this is the only app requiring a timeout to avoid a "weird flicker"; |
There was a problem hiding this comment.
Could you please open an issue for that (if none exist yet), this seems like a good candidate for tech debt work.
There was a problem hiding this comment.
Good idea. Done (#121386)
|
Thanks for the review @yuliacech ! 👍 |
Co-authored-by: Kibana Machine <[email protected]>
In this PR I have fixed the rollup CIT tests which were still using
timeoutand notjest.useFakeTimers. I've also remove all use ofnextTickin the different test files.Fixes #70043
Fixes #69783